fix(portals-admin): signees filters #16211
Conversation
WalkthroughThe pull request introduces multiple changes across various files in the signature collection module, focusing on localization improvements, updates to rendering logic, and adjustments to component props. New message definitions are added for better user guidance, while existing messages are refined for clarity. The components are modified to conditionally display content based on new props, enhancing the overall functionality and user experience. Changes
Possibly related PRs
Suggested reviewers
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (18)
libs/portals/admin/signature-collection/src/shared-components/listInfoAlert/index.tsx (1)
11-11: LGTM! Minor UI adjustment approved.The change from
marginBottom={5}tomarginBottom={3}is a small UI adjustment that reduces the spacing below the alert message. This modification adheres to the coding guidelines for files in thelibsdirectory:
- The component maintains its reusability across different NextJS apps.
- TypeScript is correctly used for defining props and types.
- The import statement supports effective tree-shaking and bundling.
Consider documenting this spacing change in the component's comments or in a UI style guide to maintain consistency across the application.
libs/portals/admin/signature-collection/src/shared-components/signees/sortSignees/index.tsx (2)
Line range hint
1-53: Component adheres to coding guidelines. Consider named export for better tree-shaking.The
SortSigneescomponent adheres well to the coding guidelines forlibs/**/*files:
- It's reusable across different NextJS apps due to its self-contained nature.
- TypeScript is used effectively for defining props and internal types.
- The component is likely tree-shakable as it doesn't have side effects.
For even better tree-shaking, consider using a named export instead of a default export:
-export default SortSignees +export { SortSignees }This change allows bundlers to more easily identify and remove unused exports.
Line range hint
18-53: Consider extracting sorting logic into a separate utility function.While the current implementation is functional, extracting the sorting logic into a separate utility function could enhance readability, maintainability, and testability. This would also make the sorting functionality more reusable across different components if needed.
Consider creating a separate utility file for sorting functions:
// sortUtils.ts import { SignatureCollectionSignature as Signature } from '@island.is/api/schema' export const sortByName = (a: Signature, b: Signature, ascending: boolean = true) => { return ascending ? a.signee.name.localeCompare(b.signee.name) : b.signee.name.localeCompare(a.signee.name) } export const sortByDate = (a: Signature, b: Signature, ascending: boolean = true) => { return ascending ? a.created.localeCompare(b.created) : b.created.localeCompare(a.created) }Then, update the
SortSigneescomponent to use these utility functions:import { sortByName, sortByDate } from './sortUtils' // ... (in the SortSignees component) items={[ createSortItem(formatMessage(m.sortAlphabeticallyAsc), (a, b) => sortByName(a, b)), createSortItem(formatMessage(m.sortAlphabeticallyDesc), (a, b) => sortByName(a, b, false)), createSortItem(formatMessage(m.sortDateAsc), (a, b) => sortByDate(a, b)), createSortItem(formatMessage(m.sortDateDesc), (a, b) => sortByDate(a, b, false)), ]}This refactoring would make the code more modular and easier to maintain.
libs/portals/admin/signature-collection/src/lib/utils.ts (1)
14-17: LGTM! Consider enhancing internationalization.The
signeeTypesconstant is well-structured and follows TypeScript best practices. It's likely to be reusable across different components, which aligns with our coding guidelines.To improve internationalization, consider using a translation function for the labels:
export const signeeTypes = [ { value: 'paper', label: t('signeeTypes.paper') }, { value: 'digital', label: t('signeeTypes.digital') }, ]This approach would make it easier to support multiple languages in the future.
libs/portals/admin/signature-collection/src/shared-components/signees/filterSignees/index.tsx (1)
60-88: LGTM with a minor optimization suggestion.The FilterMultiChoice component is well-implemented:
- Provides appropriate categories for filtering (signeeType and pageNumber).
- Uses correct callbacks to update filters based on user interactions.
- Maintains immutability of the filters object using the spread operator.
Consider simplifying the onChange callback:
onChange={(event) => onSetFilters({ ...filters, [event.categoryId]: event.selected })}This change reduces the callback to a single line, making it more concise without affecting functionality.
libs/portals/admin/signature-collection/src/shared-components/extendDeadline/index.tsx (2)
66-66: LGTM! Consider using a design token for consistency.The increased margin improves the spacing between the input field and the calendar button. For better maintainability and consistency across the application, consider using a design token instead of a hard-coded value.
You could replace the hard-coded value with a design token:
-<Box marginLeft={3}> +<Box marginLeft="spacing3">This assumes that your design system has a
spacing3token. Adjust the token name according to your actual design system.
Line range hint
1-108: Enhance type definitions and exports for better reusabilityThe component generally adheres to the coding guidelines for
libs/**/*files. However, there are a few improvements we can make to enhance type safety and reusability:
- Define and export an interface for the component props.
- Use the defined interface for the component props.
- Export the component as a named export instead of a default export.
Here's how you can implement these improvements:
- Add the following interface definition at the top of the file:
export interface ActionExtendDeadlineProps { listId: string; endTime: string; allowedToProcess?: boolean; }
- Update the component definition:
const ActionExtendDeadline: React.FC<ActionExtendDeadlineProps> = ({ listId, endTime, allowedToProcess, }) => { // ... component body ... }
- Change the export statement at the end of the file:
export { ActionExtendDeadline };These changes will improve type safety, make the component more easily importable, and enhance its reusability across different NextJS apps.
libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (2)
49-51: Approve localization change and suggest robust title handlingThe use of
parliamentaryMessages.signatureListsTitlefor localization is a good practice. However, the splitting of the list title might be fragile.Consider using a more robust method to extract the main title, such as:
title: list.title.split(' - ')[0].trim()This ensures that any trailing whitespace is removed, making it more resilient to formatting changes.
Also applies to: 61-61
Clarify the message for the 'Inactive' status
The 'Inactive' status is currently using the same message as the 'Reviewed' status, which might confuse users. It is recommended to add a specific message for the 'Inactive' status to ensure clarity, or document the reasoning behind this decision.
🔗 Analysis chain
Line range hint
71-81: Approve status message updates and request clarificationThe updates to the ListInfo component improve the clarity of status messages and ensure proper localization through consistent use of
formatMessage.Could you please clarify why the 'Inactive' status uses the same message as the 'Reviewed' status? This might lead to confusion for users. Consider adding a specific message for the 'Inactive' status or document the reasoning behind this decision.
To verify the impact of this change, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of ListStatus.Inactive to ensure consistent handling rg --type typescript "ListStatus\.Inactive" -C 5Length of output: 85
Script:
#!/bin/bash # Search for other occurrences of ListStatus.Inactive to ensure consistent handling rg --type ts "ListStatus\.Inactive" -C 5Length of output: 4879
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (1)
98-98: Standardized date formattingThe change to use
format(new Date(list.endTime), 'dd.MM.yyyy HH:mm')provides a more consistent and standardized date format. This approach is locale-independent and uses the efficiently importedformatfunction from date-fns.Consider extracting this date formatting logic into a separate utility function for better reusability across the application. This would align with the guideline of promoting reusability of components and hooks.
// In a separate utility file import { format } from 'date-fns'; export const formatDateTime = (date: Date | string): string => { return format(new Date(date), 'dd.MM.yyyy HH:mm'); }; // In your component import { formatDateTime } from '../../utils/dateUtils'; // ... date={formatDateTime(list.endTime)}libs/portals/admin/signature-collection/src/shared-components/signees/index.tsx (4)
32-36: LGTM: New filters state for enhanced functionality.The addition of the
filtersstate variable withFiltersSigneeTypetype is well-implemented and enhances the component's filtering capabilities. It follows TypeScript best practices and is correctly initialized.Consider adding a comment explaining the structure and purpose of the
filtersstate for better code readability:// State to manage active filters for signee type and page number const [filters, setFilters] = useState<FiltersSigneeType>({ signeeType: [], pageNumber: [], })
41-48: LGTM: Enhanced filtering logic.The updated
filteredSigneeslogic successfully incorporates the newsigneeTypeandpageNumberfilters while maintaining the existing search functionality. The implementation is clear and effective.Consider extracting the filter conditions into separate functions for improved readability and maintainability:
const matchesSigneeType = (s: Signature) => filters.signeeType.length === 0 || (filters.signeeType.includes('digital') && s.isDigital) || (filters.signeeType.includes('paper') && !s.isDigital); const matchesPageNumber = (s: Signature) => filters.pageNumber.length === 0 || filters.pageNumber.includes(String(s.pageNumber)); const matchesSearchTerm = (s: Signature) => s.signee.name.toLowerCase().includes(lowercaseSearchTerm) || formatNationalId(s.signee.nationalId).includes(searchTerm) || s.signee.nationalId.includes(searchTerm); return allSignees.filter((s) => matchesSigneeType(s) && matchesPageNumber(s) && matchesSearchTerm(s) );This refactoring improves code organization and makes each filter condition more self-contained and easier to understand.
Line range hint
63-98: LGTM: Improved layout with new filtering components.The updated layout successfully incorporates the new SortSignees and FilterSignees components, enhancing the UI's filtering capabilities. The use of GridColumn and Box components maintains consistency with the project's UI framework and preserves responsive design.
For consistency in spacing, consider using the
spacingprop of the Box component instead of marginLeft:<Box display="flex" justifyContent="spaceBetween" marginTop={[2, 2, 2, 0]} spacing={1} > <SortSignees signees={signees} setSignees={setSignees} setPage={setPage} /> <FilterSignees signees={signees} filters={filters} onSetFilters={setFilters} /> </Box>This change would make the spacing between SortSignees and FilterSignees more consistent with the project's styling approach.
132-138: LGTM: Enhanced visual distinction for signee types.The addition of conditional background colors for T.Data components effectively distinguishes between digital and paper signees. The consistent application across all columns and the explicit width setting for the date column improve the overall layout.
To improve code reusability and reduce repetition, consider extracting the common props into a separate object:
const getCommonDataProps = (isDigital: boolean) => ({ text: { variant: 'medium' }, box: { background: isDigital ? 'white' : 'blueberry100', }, }); // Usage: <T.Data {...getCommonDataProps(s.isDigital)} style={{ width: '25%' }} > {format(new Date(s.created), 'dd.MM.yyyy HH:mm')} </T.Data>This refactoring would make the code more DRY and easier to maintain.
Also applies to: 141-146, 149-154, 157-161
libs/portals/admin/signature-collection/src/screens-presidential/AllLists/index.tsx (3)
25-25: LGTM! Consider adding type documentation.The renaming of
FilterstoFiltersOverviewimproves clarity. This change aligns well with TypeScript best practices.Consider adding JSDoc comments to the
FiltersOverviewtype to provide more context about its purpose and structure. This would enhance code readability and maintainability.
51-54: LGTM! Consider using consistent naming conventions.The new filter structure using
FiltersOverviewtype improves type safety and readability. It's a good practice to use explicit typing for state variables in React components.For consistency, consider using camelCase for all filter properties. Change 'area' to 'countryArea' to match the
countryAreasvariable used elsewhere in the code. This would make the code more uniform and easier to maintain.
Line range hint
86-106: LGTM! Consider further optimization.The updated logic for setting candidates and checking for lists in review is more efficient and concise. Combining these operations in a single iteration improves performance.
Consider using
Array.prototype.reduceto further optimize this operation. This would allow you to create the candidates array and set thehasInReviewstate in a single pass, potentially improving performance for large lists. Here's an example:useEffect(() => { if (lists.length > 0) { const { candidates, hasInReview } = lists.reduce( (acc, list) => { if (!list.reviewed) { acc.hasInReview = true; } if (!acc.candidateSet.has(list.candidate.name)) { acc.candidateSet.add(list.candidate.name); acc.candidates.push({ label: list.candidate.name, value: list.candidate.name, }); } return acc; }, { candidates: [], candidateSet: new Set(), hasInReview: false } ); setCandidates(candidates); setHasInReview(hasInReview); } }, []);This approach eliminates the need for the
filterand secondmapoperation, potentially improving performance for larger datasets.libs/portals/admin/signature-collection/src/lib/messages.ts (1)
Line range hint
577-580: Corrected message ID for paper number edit errorThe message ID has been correctly updated from "editPaperNumberSuccess" to "editPaperNumberError", fixing a copy-paste mistake. This correction is crucial for proper error handling and display.
However, to further improve code quality and prevent similar issues in the future, consider using a consistent naming convention for success and error messages.
Consider applying this naming pattern consistently across all success/error message pairs:
editPaperNumberSuccess: { id: 'admin-portal.signature-collection:editPaperNumberSuccess', defaultMessage: 'Tókst að breyta blaðsíðunúmeri', description: '', }, editPaperNumberError: { - id: 'admin-portal.signature-collection:editPaperNumberSuccess', + id: 'admin-portal.signature-collection:editPaperNumberError', defaultMessage: 'Ekki tókst að breyta blaðsíðunúmeri', description: '', },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
- libs/portals/admin/signature-collection/src/lib/messages.ts (5 hunks)
- libs/portals/admin/signature-collection/src/lib/utils.ts (1 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (3 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (2 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (2 hunks)
- libs/portals/admin/signature-collection/src/screens-presidential/AllLists/index.tsx (2 hunks)
- libs/portals/admin/signature-collection/src/shared-components/extendDeadline/index.tsx (1 hunks)
- libs/portals/admin/signature-collection/src/shared-components/listInfoAlert/index.tsx (1 hunks)
- libs/portals/admin/signature-collection/src/shared-components/signees/filterSignees/index.tsx (1 hunks)
- libs/portals/admin/signature-collection/src/shared-components/signees/index.tsx (4 hunks)
- libs/portals/admin/signature-collection/src/shared-components/signees/sortSignees/index.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
libs/portals/admin/signature-collection/src/lib/messages.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/signature-collection/src/lib/utils.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/signature-collection/src/screens-presidential/AllLists/index.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/signature-collection/src/shared-components/extendDeadline/index.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/signature-collection/src/shared-components/listInfoAlert/index.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/signature-collection/src/shared-components/signees/filterSignees/index.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/signature-collection/src/shared-components/signees/index.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/signature-collection/src/shared-components/signees/sortSignees/index.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (19)
libs/portals/admin/signature-collection/src/lib/utils.ts (1)
25-28: LGTM! Well-structured type definition.The
FiltersSigneeTypetype is well-defined and follows TypeScript best practices. It's exported, making it reusable across different components, which aligns with our coding guidelines.The structure with
signeeTypeandpageNumberas string arrays provides flexibility for filtering operations. Good job on maintaining type safety and clarity.libs/portals/admin/signature-collection/src/shared-components/signees/filterSignees/index.tsx (3)
1-16: LGTM: Imports and component declaration adhere to guidelines.The imports and component declaration follow best practices:
- TypeScript is used to define props, enhancing type safety.
- The component is exported, promoting reusability across different NextJS apps.
- Imports are appropriately organized, including necessary UI components and utilities.
43-59: LGTM: Proper use of UI components and localization.The Filter component setup demonstrates good practices:
- Utilizes the @island.is/island-ui/core library components effectively.
- Implements localization correctly using the useLocale hook.
- Provides a clear onFilterClear callback to reset all filters.
This approach ensures consistency with the UI library and supports internationalization efforts.
94-94: LGTM: Component export follows best practices.The default export of the FilterSignees component adheres to coding guidelines:
- Allows for easy import and reuse across different NextJS apps.
- Follows the convention of exporting the main component of the file as the default export.
libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (1)
69-69: Approve UI spacing enhancementThe addition of
marginBottom={3}to the IntroHeader component improves the layout spacing, enhancing the overall user interface.This change aligns with the guideline for creating reusable components across different NextJS apps by allowing for flexible spacing adjustments.
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (2)
48-50: LGTM: Improved localization for breadcrumb titleThe change from a hardcoded string to
parliamentaryMessages.signatureListsTitleenhances localization and maintains consistency with the main title. This adheres to best practices for internationalization.
72-72: Verify search functionality alignment with new placeholderThe placeholder change from
m.searchInListPlaceholdertom.searchNationalIdPlaceholdersuggests a shift to national ID-based searching. This change appears appropriate for a signature collection context.Please ensure that:
- The search functionality has been updated to handle national ID searches.
- This change aligns with the intended user experience.
To verify the search functionality, you can run the following script:
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (3)
22-22: Good use of tree-shaking with date-fns importThe import of
formatfromdate-fnsadheres to effective tree-shaking practices. This approach ensures that only the necessary function is imported, potentially reducing the bundle size.
56-58: Improved localization for breadcrumb titleThe update to use
parliamentaryMessages.signatureListsTitleinstead of a hardcoded string is a good practice for localization. This change enhances the reusability of the component across different NextJS apps and makes it easier to maintain consistent translations.
Line range hint
1-158: Overall assessment: Improvements in localization and consistencyThe changes made to this file enhance localization and consistency, particularly in the breadcrumb title and date formatting. The code adheres well to the guidelines for
libsdirectory files, demonstrating good practices in reusability, TypeScript usage, and tree-shaking.To further improve:
- Consider extracting the date formatting logic into a utility function for better reusability.
- Ensure that all strings visible to users are using message keys for consistent localization.
The component remains well-structured and maintainable after these changes.
libs/portals/admin/signature-collection/src/shared-components/signees/index.tsx (3)
17-18: LGTM: New imports for filtering functionality.The new imports for
SortSignees,FiltersSigneeType, andpageSizeare appropriate for the added filtering functionality. They follow TypeScript practices and contribute to the component's reusability.
21-21: LGTM: Import for FilterSignees component.The import of the
FilterSigneescomponent aligns with the new filtering functionality and follows the component reusability pattern.
Line range hint
1-214: Great job on enhancing the Signees component!The changes to the Signees component significantly improve its functionality by adding robust filtering capabilities. The implementation adheres to the coding guidelines for
libs/**/*files:
- Reusability: The component remains highly reusable across different NextJS apps.
- TypeScript usage: Prop types and state management consistently use TypeScript, enhancing type safety.
- Effective tree-shaking and bundling: No issues are apparent in this regard.
The new filtering features, including SortSignees and FilterSignees, are well-integrated into the existing component structure. The conditional styling for digital and paper signees improves visual clarity.
Overall, this is a solid enhancement that maintains code quality while significantly improving user experience.
libs/portals/admin/signature-collection/src/lib/messages.ts (6)
80-82: Improved clarity for filter labelThe change from "Opna síu" to "Sía" makes the label more concise and aligns better with common UI terminology. This improvement enhances user understanding and maintains consistency across the interface.
113-117: New message for signee type categorizationThe addition of the
typeOfSigneemessage enhances the user interface by providing a clear label for categorizing different types of signees. This improvement will help in organizing and filtering the signature collection data more effectively.
136-137: Corrected typo in collection creation modal descriptionThe spelling error in "frambjóðs" has been corrected to "framboðs", which improves the accuracy and professionalism of the user interface. This correction ensures that the message conveys the intended meaning without any confusion.
159-163: New message for editing list end timeThe addition of the
editListEndTimemessage enhances the user interface by providing a clear label for the action of modifying the end time of a list. This new feature will improve the flexibility in managing signature collections, allowing administrators to adjust deadlines as needed.
309-311: Simplified sorting labelThe change from "Raða eftir" to "Raða" for the sorting label makes the interface more concise while maintaining clarity. This adjustment aligns with modern UI design principles, favoring shorter, more direct labels that reduce cognitive load for users.
302-306: New placeholder for national ID searchThe addition of the
searchNationalIdPlaceholdermessage enhances the user experience by providing clear guidance for the national ID search feature. This specific placeholder will help users understand the expected input format and purpose of the search field, potentially reducing errors and improving search efficiency.
libs/portals/admin/signature-collection/src/shared-components/signees/sortSignees/index.tsx
Show resolved
Hide resolved
libs/portals/admin/signature-collection/src/shared-components/signees/filterSignees/index.tsx
Outdated
Show resolved
Hide resolved
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx (2)
Line range hint
1-93: Consider extracting mutation logic into a custom hookTo improve the separation of concerns and enhance reusability, consider extracting the
toggleReviewfunction and related state into a custom hook. This would make the component more focused on rendering and improve testability.Example:
const useToggleReview = (listId: string) => { const [toggleListReviewMutation, { loading }] = useToggleListReviewMutation(); const { revalidate } = useRevalidator(); const toggleReview = async () => { // ... existing logic ... }; return { toggleReview, loading }; };Then use it in the component:
const { toggleReview, loading } = useToggleReview(listId);
Line range hint
18-22: Handle potential undefinedlistStatusThe
listStatusprop is optional, but the component assumes it exists when settinglistReviewed. To prevent potential issues, consider handling the case wherelistStatusis undefined.Suggestion:
const listReviewed = listStatus === ListStatus.Reviewed;This change ensures that
listReviewedis always a boolean, even iflistStatusis undefined.libs/portals/admin/signature-collection/src/module.tsx (5)
35-41: LGTM with a minor suggestion for optimizationThe addition of the
allowedToProcessprop to theParliamentaryRootcomponent is well-implemented and consistent with TypeScript best practices. It enhances the reusability of the component across different NextJS apps by passing down permissions as a prop.Consider extracting the scope checking logic into a separate function to improve readability and reduce duplication across route definitions. For example:
const isAllowedToProcess = (scopes: string[]) => scopes.includes(AdminPortalScope.signatureCollectionProcess); // Usage allowedToProcess={isAllowedToProcess(props.userInfo.scopes)}This would make the code more DRY and easier to maintain.
59-65: LGTM - Consider refactoring for DRY principleThe addition of the
allowedToProcessprop to theParliamentaryListcomponent maintains consistency with the previous implementations. It adheres to TypeScript best practices and supports component reusability.As this is the third occurrence of the same scope checking logic, it's becoming increasingly important to refactor this repeated code. Implementing a shared function as suggested earlier would significantly improve code maintainability and adhere better to the DRY (Don't Repeat Yourself) principle. This refactoring would make the code more efficient and less prone to errors if the logic needs to be updated in the future.
Line range hint
71-77: LGTM - Refactoring neededThe addition of the
allowedToProcessprop to theAllListscomponent maintains consistency with the previous implementations. It continues to adhere to TypeScript best practices and supports component reusability.This is now the fourth occurrence of the same scope checking logic. The need for refactoring this repeated code is critical at this point. Implementing a shared function for this logic would significantly improve code maintainability, reduce the likelihood of errors, and make future updates more straightforward. This refactoring is essential for adhering to best practices in code organization and the DRY principle.
Line range hint
83-89: LGTM - Urgent refactoring recommendedThe addition of the
allowedToProcessprop to theListcomponent maintains consistency with all previous implementations. It adheres to TypeScript best practices and supports component reusability.This is the fifth occurrence of the identical scope checking logic, which strongly emphasizes the urgent need for refactoring. To significantly improve code quality, maintainability, and adherence to the DRY principle, I strongly recommend implementing a shared function for this logic. Here's a suggested implementation:
const isAllowedToProcess = (scopes: string[]) => scopes.includes(AdminPortalScope.signatureCollectionProcess); // Then use it in all route definitions like this: allowedToProcess={isAllowedToProcess(props.userInfo.scopes)}This refactoring will:
- Reduce code duplication
- Improve maintainability
- Make future updates easier and less error-prone
- Enhance code readability
Please consider implementing this refactoring before merging the pull request.
Line range hint
1-89: Overall assessment: Approved with recommendations for improvementThe changes introduced in this file consistently implement the
allowedToProcessprop across various components, adhering to TypeScript best practices and supporting component reusability. This enhancement improves the functionality by adding permission checks based on user scopes.Key points:
- The code adheres to the coding guidelines for files in the
libsdirectory.- The changes support reusability across different NextJS apps.
- TypeScript is used effectively for defining props.
While the implementation is functionally correct, there's a significant opportunity for improvement by addressing the repeated scope checking logic. I strongly recommend refactoring this repeated code into a shared function before merging the pull request. This refactoring will enhance code quality, maintainability, and adherence to the DRY principle, making the codebase more robust and easier to maintain in the long run.
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (2)
23-27: LGTM: Component signature updated with new propThe
ParliamentaryRootcomponent now accepts anallowedToProcessprop of type boolean, which enhances its reusability. The prop is correctly typed with TypeScript, adhering to the coding guidelines.Consider adding a default value for the
allowedToProcessprop to improve the component's usability:const ParliamentaryRoot = ({ allowedToProcess = false, }: { allowedToProcess?: boolean }) => { // ... }This change would make the prop optional and provide a default behavior.
66-68: LGTM: Conditional logo rendering implementedThe
IntroHeadercomponent now conditionally renders the appropriate logo based on theallowedToProcessprop. This implementation correctly uses the newly imported SVG assets and aligns with the updated component signature.Consider extracting the logo selection logic into a separate function or variable for improved readability:
const logoSrc = allowedToProcess ? electionsCommitteeLogo : nationalRegistryLogo; // ... <IntroHeader // ... img={logoSrc} />This change would make the JSX more concise and the logic more explicit.
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (2)
26-30: LGTM: Component signature updated with TypeScript prop definition.The addition of the
allowedToProcessprop with TypeScript typing is good. It aligns with the coding guidelines for TypeScript usage in defining props.Consider using an interface for the props to improve reusability and maintainability:
interface ConstituencyProps { allowedToProcess: boolean; } export const Constituency = ({ allowedToProcess }: ConstituencyProps) => { // ... }
83-85: LGTM: Enhanced logo rendering and improved date formatting.The conditional rendering of logos based on
allowedToProcessand the use of date-fns for date formatting are good improvements. They enhance the component's flexibility and consistency.Consider extracting the date format string to a constant for better maintainability:
const DATE_FORMAT = 'dd.MM.yyyy HH:mm'; // ... date={format(new Date(list.endTime), DATE_FORMAT)}Also applies to: 108-108
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
libs/portals/admin/signature-collection/assets/electionsCommittee.svgis excluded by!**/*.svglibs/portals/admin/signature-collection/assets/nationalRegistry.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
- libs/portals/admin/signature-collection/src/module.tsx (1 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (4 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (4 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (4 hunks)
- libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx (1 hunks)
- libs/portals/admin/signature-collection/src/shared-components/signees/filterSignees/index.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx
- libs/portals/admin/signature-collection/src/shared-components/signees/filterSignees/index.tsx
🧰 Additional context used
📓 Path-based instructions (4)
libs/portals/admin/signature-collection/src/module.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (7)
libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx (1)
81-81: Excellent improvement to UI consistency!The addition of the
iconprop to the Button component inside the Modal enhances the user experience by providing consistent visual feedback. This change aligns well with the component's purpose and improves its reusability across different parts of the application.libs/portals/admin/signature-collection/src/module.tsx (1)
47-53: LGTM - Consistent implementationThe addition of the
allowedToProcessprop to theParliamentaryConstituencycomponent is consistent with the previous implementation forParliamentaryRoot. It maintains good TypeScript practices and component reusability.The same optimization suggestion from the previous comment applies here as well. Implementing a shared function for checking the scope would improve consistency and maintainability across all route definitions.
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (3)
20-21: LGTM: New logo imports added correctlyThe new SVG imports for
electionsCommitteeLogoandnationalRegistryLogoare correctly added and follow proper naming conventions. These imports support the new conditional rendering based on theallowedToProcessprop.
54-56: LGTM: Breadcrumbs title updated for better localizationThe Breadcrumbs title now uses a localized message from
parliamentaryMessages, improving the component's internationalization support. TheformatMessagefunction is used correctly, and the code style is consistent with the rest of the file.
81-81: LGTM: FilterInput placeholder updatedThe placeholder text for the FilterInput has been updated to use
m.searchNationalIdPlaceholder, which suggests a more specific placeholder for national ID search. This change improves the user experience by providing clearer guidance.Reminder: The TODO comment for implementing search functionality is still present. Please ensure that the search functionality is implemented as mentioned in the previous review comment.
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (2)
22-24: LGTM: New imports enhance functionality and visual elements.The addition of
formatfrom 'date-fns' and the logo imports are well-aligned with the component's updated requirements. These specific imports support effective tree-shaking, which is good for bundling practices.
63-65: LGTM: Improved localization for breadcrumb title.The use of
parliamentaryMessages.signatureListsTitlefor the breadcrumb title is a good improvement. It enhances maintainability and supports multiple languages, aligning with localization best practices.
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16211 +/- ##
=======================================
Coverage 36.96% 36.96%
=======================================
Files 6778 6778
Lines 139856 139856
Branches 39769 39769
=======================================
Hits 51697 51697
Misses 88159 88159
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
|
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx
Outdated
Show resolved
Hide resolved
libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx
Outdated
Show resolved
Hide resolved
albinagu
added
deprecated:automerge
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- libs/portals/admin/signature-collection/src/lib/messages.ts (6 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (5 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/portals/admin/signature-collection/src/lib/messages.ts
- libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx
🧰 Additional context used
📓 Path-based instructions (1)
libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (6)
libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (6)
11-11: Good use of localized messagesImporting
parliamentaryMessagesenhances localization and maintainability. This change helps in keeping the messages organized and consistent throughout the application.
19-20: Importing logos for conditional renderingImporting
electionsCommitteeLogoandnationalRegistryLogoallows for appropriate branding based on theallowedToProcessprop, improving user experience.
51-53: Consistent Localization in BreadcrumbsUpdating the breadcrumb title to use
parliamentaryMessages.signatureListsTitleensures consistent localization and adheres to best practices for internationalization.
69-73: Effective Conditional Rendering of Intro MessageGood use of the
allowedToProcessprop to conditionally render the introductory message. This enhances user experience by displaying relevant content based on permissions.
76-78: Conditional Rendering of Header ImageAppropriate use of
allowedToProcessto determine which logo to display in the header. This provides visual cues aligned with user permissions.
97-97: Passing 'listStatus' Prop to 'ActionReviewComplete'Adding the
listStatusprop to theActionReviewCompletecomponent enhances its functionality by allowing it to react according to the current status of the list.
libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx
Show resolved
Hide resolved
libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx
Show resolved
Hide resolved
albinagu
added
the
deprecated:automerge
kodiakhq
bot
removed
the
deprecated:automerge
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
albinagu
added
the
deprecated:automerge
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
UI/UX Enhancements