fix(portals-admin): tweaks lks vs þí #16484
Conversation
WalkthroughThe pull request introduces several modifications across multiple components in the signature collection module, primarily focusing on conditional rendering based on the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Outside diff range and nitpick comments (10)
libs/portals/admin/signature-collection/src/shared-components/extendDeadline/index.tsx (2)
66-75: Approved: Good implementation of conditional renderingThe addition of conditional rendering based on the
allowedToProcessprop improves the control flow and aligns with the expected behavior. This change enhances the component's reusability and adheres to the coding guidelines.Consider adding a comment explaining the purpose of the
allowedToProcesscheck for better code clarity:+ {/* Only show the change deadline button if the user is allowed to process */} {allowedToProcess && ( <Box marginLeft={3}> <Button icon="calendar" iconType="outline" variant="ghost" onClick={() => setModalChangeDateIsOpen(true)} ></Button> </Box> )}
Line range hint
30-46: Suggestion: Improve error handling in extendDeadline functionThe current implementation of the
extendDeadlinefunction has good error handling for API errors, but it could be improved to handle network errors more gracefully.Consider updating the error handling to differentiate between API errors and network errors:
const extendDeadline = async (newEndDate: string) => { try { const res = await extendDeadlineMutation({ variables: { input: { listId, newEndDate: newEndDate, }, }, }); if (res.data?.signatureCollectionAdminExtendDeadline.success) { toast.success(formatMessage(m.updateListEndTimeSuccess)); revalidate(); } else { toast.error(formatMessage(m.updateListEndTimeError)); } } catch (e) { if (e.networkError) { toast.error(formatMessage(m.networkError)); } else { toast.error(formatMessage(m.unexpectedError)); } console.error('Error extending deadline:', e); } };This change will provide more specific error messages to the user and log the full error for debugging purposes.
libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (2)
94-98: LGTM! Consider adding type annotation forallowedToProcess.The addition of the
allowedToProcessprop to theActionExtendDeadlinecomponent aligns well with the updated method signature and enhances permission-based control. This change improves the reusability of the component across different contexts.For improved type safety, consider adding a type annotation for the
allowedToProcessprop:<ActionExtendDeadline listId={list.id} endTime={list.endTime} allowedToProcess={allowedToProcess as boolean} />
100-105: LGTM! Consider extracting conditional content to a separate component.The conditional rendering of
PaperSigneesandActionReviewCompletecomponents based on theallowedToProcessprop is a good practice. It effectively restricts access to these components for users without the necessary permissions.To improve code readability and maintainability, consider extracting the conditional content into a separate component:
const ProcessingActions = ({ listId, listStatus }) => ( <Box> <PaperSignees listId={listId} /> <ActionReviewComplete listId={listId} listStatus={listStatus} /> </Box> ); // In the main component: {allowedToProcess && <ProcessingActions listId={list.id} listStatus={listStatus} />}This refactoring would enhance the component's modularity and make it easier to manage these conditional elements in the future.
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (3)
104-109: Approve conditional rendering of DownloadReportsThe conditional rendering of the
DownloadReportscomponent based on theallowedToProcessprop enhances the control flow and improves the component's reusability. This change aligns well with the PR objectives.Consider extracting the
DownloadReportscomponent into a separate variable for improved readability:const downloadReportsComponent = allowedToProcess && ( <DownloadReports areas={collection.areas} collectionId={collection?.id} /> ); // Then in the JSX: {downloadReportsComponent}This approach can make the JSX structure cleaner, especially if more conditional components are added in the future.
207-207: Approve conditional rendering of CompareListsThe conditional rendering of the
CompareListscomponent based on theallowedToProcessprop is consistent with the previous changes and enhances the control flow. This modification improves the component's reusability across different NextJS apps with varying permission levels.For consistency with the
DownloadReportscomponent, consider using a similar structure:{allowedToProcess && ( <CompareLists collectionId={collection?.id} /> )}This approach maintains a consistent style throughout the component for conditional rendering.
Line range hint
1-210: Summary of changes and their impactThe modifications in this file effectively implement conditional rendering for the
DownloadReportsandCompareListscomponents based on theallowedToProcessprop. These changes align well with the PR objectives and coding guidelines by:
- Enhancing control flow based on user permissions.
- Improving component reusability across different NextJS apps.
- Adhering to TypeScript usage for props.
The changes contribute to a more flexible and permission-aware UI, allowing for better control over which components are displayed to users with different access levels.
Consider creating a custom hook (e.g.,
usePermissionAwareComponent) to encapsulate this permission-based rendering logic. This could further improve reusability and consistency across the application:const usePermissionAwareComponent = ( allowedToProcess: boolean, Component: React.ComponentType<any>, props: any ) => { return allowedToProcess ? <Component {...props} /> : null; }; // Usage in the component: const downloadReports = usePermissionAwareComponent( allowedToProcess, DownloadReports, { areas: collection.areas, collectionId: collection?.id } ); // Then in the JSX: {downloadReports}This approach could be particularly beneficial if you find yourself implementing similar permission-based rendering in multiple components.
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (3)
130-132: Approve conditional rendering with a minor suggestion.The conditional rendering of the
CreateCollectioncomponent based onallowedToProcessand the presence of constituency lists is a good implementation. It aligns with the PR objective of controlling access to functionalities based on user permissions.Consider extracting the condition into a separate variable for improved readability:
const canCreateCollection = allowedToProcess && constituencyLists?.length > 0; {canCreateCollection && <CreateCollection collectionId={collection?.id} />}This change would make the code more self-documenting and easier to maintain.
159-232: Approve changes with refactoring suggestion for improved readability.The conditional rendering of the "Cancel collection" dialog based on
allowedToProcessand the refactored logic for handling different scenarios are good improvements. They align with the PR objective of controlling access to functionalities based on user permissions.However, the nested ternary operators and conditions make the code harder to read and maintain. Consider refactoring this part to improve readability:
const renderCancelCollectionTag = () => { if (list.reviewed) { return { label: m.confirmListReviewed.defaultMessage, variant: 'mint', outlined: false, }; } if (!allowedToProcess) { return undefined; } return { label: 'Cancel collection', renderTag: () => ( <DialogPrompt // ... (rest of the DialogPrompt props) /> ), }; }; // In the JSX tag={renderCancelCollectionTag()}This refactoring separates the logic into a function, making it easier to understand and maintain. It also removes the nested ternary operators, improving code readability.
200-211: Approve the additional logic for removing the last candidate with a suggestion.The new condition to check if the candidate being removed is the last one associated with the list, and the subsequent
removeCandidatemutation call, is a good addition. This change improves data consistency by ensuring that candidates are removed when their last list is deleted.Consider adding error handling for the
removeCandidatemutation:removeCandidate({ variables: { input: { candidateId: list.candidate.id, }, }, }) .catch(error => { console.error('Failed to remove candidate:', error); toast.error(formatMessage(m.removeCandidateError)); });This addition would provide better feedback to the user and log any errors for debugging purposes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (2 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (1 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (2 hunks)
- libs/portals/admin/signature-collection/src/shared-components/extendDeadline/index.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/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."
🔇 Additional comments (1)
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (1)
192-197: Approve the correction in the removeList mutation.The change to use
list.idas the input for theremoveListmutation is correct. This ensures that the proper list ID is passed to the mutation, improving the accuracy of the operation.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16484 +/- ##
==========================================
- Coverage 36.79% 36.78% -0.01%
==========================================
Files 6845 6845
Lines 141716 141714 -2
Branches 40363 40361 -2
==========================================
- Hits 52138 52136 -2
Misses 89578 89578
Flags with carried forward coverage won't be shown. Click here to find out more. see 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/index.tsx (1)
Line range hint
1-76: Suggestion: Consider optimizations for better tree-shakingThe component generally follows good practices, but consider the following optimizations:
- Use named imports for
island-ui/corecomponents to potentially improve tree-shaking.- Consider memoizing the component or its child components if they're used in a list or frequently re-rendered.
Example of named imports:
import { Box, Button, Text } from '@island.is/island-ui/core'These changes could lead to better bundle size optimization and improved performance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (1 hunks)
- libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx (2 hunks)
- libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/index.tsx (2 hunks)
- libs/portals/admin/signature-collection/src/shared-components/extendDeadline/index.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx
- libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx
- libs/portals/admin/signature-collection/src/shared-components/extendDeadline/index.tsx
🧰 Additional context used
📓 Path-based instructions (1)
libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/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 (1)
libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/index.tsx (1)
7-7: Approved: Improved import for better reusabilityThe change to import
ListStatusfrom@island.is/api/schemainstead of a local utility file enhances reusability across different NextJS apps. This aligns with our coding guidelines and promotes consistency in the codebase.
libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/index.tsx
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/portals/admin/signature-collection/src/screens-presidential/AllLists/index.tsx (2)
268-268: LGTM: Improved tag rendering and localizationThe update to the
tagproperty in theActionCardcomponent is a good improvement. It now uses localization for the label and is conditionally rendered based on thelist.reviewedstatus. This enhances both the UI and internationalization support.Consider extracting the tag object into a separate constant for better readability:
const reviewedTag = list.reviewed ? { label: formatMessage(m.confirmListReviewed), variant: 'mint', outlined: false, } : undefined; // Then in the ActionCard props: tag={reviewedTag}
Line range hint
271-288: Approved: Enhanced permission-based rendering, but condition can be simplifiedThe conditional rendering of the
ctaproperty based onallowedToProcessandcollectionStatusis a good improvement, aligning with the PR objectives. However, the condition can be simplified for better readability.Consider simplifying the condition as follows:
cta={ (allowedToProcess && collectionStatus !== CollectionStatus.InitialActive) || !allowedToProcess ? { label: formatMessage(m.viewList), variant: 'text', icon: 'arrowForward', onClick: () => { navigate( SignatureCollectionPaths.PresidentialList.replace( ':listId', list.id, ), ) }, } : undefined }Can be simplified to:
cta={ collectionStatus !== CollectionStatus.InitialActive || !allowedToProcess ? { label: formatMessage(m.viewList), variant: 'text', icon: 'arrowForward', onClick: () => { navigate( SignatureCollectionPaths.PresidentialList.replace( ':listId', list.id, ), ) }, } : undefined }This simplification maintains the same logic but is easier to read and understand.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- libs/portals/admin/signature-collection/src/lib/messages.ts (2 hunks)
- libs/portals/admin/signature-collection/src/lib/navigation.ts (2 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (5 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (4 hunks)
- libs/portals/admin/signature-collection/src/screens-presidential/AllLists/index.tsx (1 hunks)
- libs/portals/admin/signature-collection/src/shared-components/createCollection/index.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx
- libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx
🧰 Additional context used
📓 Path-based instructions (4)
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/navigation.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-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/createCollection/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 (8)
libs/portals/admin/signature-collection/src/lib/navigation.ts (3)
3-3: LGTM: Import changes improve modularityThe change from importing
parliamentaryMessagesto importingmfrom './messages' enhances modularity and supports better tree-shaking. This aligns well with the coding guidelines for reusability and effective bundling practices.
14-14: LGTM: Navigation structure update is consistentThe change from
parliamentaryMessages.signatureListsTitletom.parliamentaryCollectionTitleis consistent with the import changes and maintains the TypeScript usage for defining props. This update supports the reusability of the navigation structure across different NextJS apps.
Line range hint
1-24: Adherence to coding guidelines confirmedThe changes in this file adhere to the coding guidelines for the
libs/directory:
- The
signatureCollectionNavigationstructure supports reusability across different NextJS apps.- TypeScript is used consistently for defining props and types.
- The import changes support effective tree-shaking and bundling practices.
These modifications maintain the quality and consistency of the codebase.
libs/portals/admin/signature-collection/src/shared-components/createCollection/index.tsx (1)
172-172: Improved button validation logicThe additional check
|| !namein the button's disabled state is a good improvement. It ensures that the button remains disabled not only whencanCreateis false but also when thenamefield is empty. This enhancement prevents the creation of collections with empty names, improving data integrity and user experience.libs/portals/admin/signature-collection/src/screens-presidential/AllLists/index.tsx (2)
Line range hint
1-1: LGTM: Prop addition enhances permission-based renderingThe addition of the
allowedToProcessprop to theListscomponent is a good improvement. It allows for more granular control over what users can see and do based on their permissions, which aligns well with the PR objectives.
Line range hint
1-388: Overall: Solid improvements to permission handling and localizationThe changes in this file significantly enhance the
Listscomponent by improving permission-based rendering, refining the UI based on collection status, and strengthening localization support. These updates align well with the PR objectives and adhere to the coding guidelines.Key improvements:
- Introduction of the
allowedToProcessprop for fine-grained permission control.- Enhanced conditional rendering in the
ActionCardcomponent.- Improved localization for the reviewed list tag.
The component maintains good reusability, makes proper use of TypeScript, and doesn't introduce any apparent issues with bundling practices. These changes contribute to a more robust and user-friendly signature collection module.
libs/portals/admin/signature-collection/src/lib/messages.ts (2)
16-16: LGTM: Consistent message ID update.The update to the
signatureListsTitlePresidentialmessage ID is consistent with the overall structure and improves organization.
169-185: LGTM: New parliamentary collection messages added.The addition of
parliamentaryCollectionTitle,parliamentaryCollectionIntro, andparliamentaryConstituencyIntromessages enhances support for parliamentary elections. These new messages follow existing patterns and conventions, improving the localization capabilities for the signature collection feature.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes