fix(service-portal): parliamentary refactor signee view #16030
Conversation
WalkthroughThe pull request introduces several modifications across multiple files within the signature collection module. Key changes include the localization of messages for Icelandic elections, the removal of unused components and hooks in the Changes
Possibly related PRs
Suggested labels
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
Early access features: enabledWe are currently testing the following features in early access:
Note:
|
![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 (2)
libs/service-portal/signature-collection/src/skeletons.tsx (1)
Line range hint
9-12: Enhance component reusability by accepting props and defining prop typesThe
SkeletonTablecomponent currently has hardcoded values forSkeletonLoaderproperties. Consider accepting these values as props to make the component more reusable across different NextJS apps. Additionally, defining prop types using TypeScript would improve type safety and clarity.Apply this diff to refactor the component:
+type SkeletonTableProps = { + height?: number +} -export const SkeletonTable = () => { +export const SkeletonTable = ({ height = 700 }: SkeletonTableProps) => { return ( <Box marginTop={5}> - <SkeletonLoader height={700} borderRadius="large" /> + <SkeletonLoader height={height} borderRadius="large" /> </Box> ) }libs/service-portal/signature-collection/src/screens/shared/SignedList/index.tsx (1)
124-146: Improve accessibility of the modal dialogTo enhance accessibility, it's important to ensure that modals are announced properly to assistive technologies.
Consider using the
dialogrole and addingaria-modalattributes to theModalcomponent:<Modal id="unSignList" isVisible={modalIsOpen} toggleClose={false} initialVisibility={false} onCloseModal={() => setModalIsOpen(false)} + aria-modal="true" + role="dialog" >Additionally, ensure that the modal has a clearly associated label, either through the
aria-labelledbyattribute or by including a heading within the modal content.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- libs/service-portal/signature-collection/src/lib/messages.ts (2 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx (1 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/modals/DeletePerson/index.tsx (0 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/modals/LookupPerson/index.tsx (0 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/SignedList/index.tsx (0 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/index.tsx (2 hunks)
- libs/service-portal/signature-collection/src/screens/Presidential/OwnerView/index.tsx (2 hunks)
- libs/service-portal/signature-collection/src/screens/Presidential/SigneeView/index.tsx (0 hunks)
- libs/service-portal/signature-collection/src/screens/Presidential/index.tsx (1 hunks)
- libs/service-portal/signature-collection/src/screens/shared/SignedList/index.tsx (4 hunks)
- libs/service-portal/signature-collection/src/screens/shared/SigneeView/index.tsx (2 hunks)
- libs/service-portal/signature-collection/src/skeletons.tsx (1 hunks)
Files not reviewed due to no reviewable changes (4)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/modals/DeletePerson/index.tsx
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/modals/LookupPerson/index.tsx
- libs/service-portal/signature-collection/src/screens/Parliamentary/SignedList/index.tsx
- libs/service-portal/signature-collection/src/screens/Presidential/SigneeView/index.tsx
Files skipped from review due to trivial changes (1)
- libs/service-portal/signature-collection/src/screens/Presidential/index.tsx
Additional context used
Path-based instructions (7)
libs/service-portal/signature-collection/src/skeletons.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/service-portal/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/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/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/service-portal/signature-collection/src/screens/shared/SigneeView/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/service-portal/signature-collection/src/screens/Presidential/OwnerView/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/service-portal/signature-collection/src/screens/shared/SignedList/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/service-portal/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."
Additional comments not posted (11)
libs/service-portal/signature-collection/src/screens/Parliamentary/index.tsx (4)
15-15: Initialization of the localization namespace is appropriateCalling
useNamespaces('sp.signatureCollection')correctly sets up the localization context for this component, enhancing the handling of translation strings.
10-10: Confirm the updated import path forSigneeViewThe
SigneeViewcomponent is now imported from'../shared/SigneeView'. Ensure that the component exists at this new location and that all references are updated accordingly.You can verify the existence of the component at the new path with the following script:
Verification successful
Import path for
SigneeViewis correct and verifiedThe
SigneeViewcomponent exists atlibs/service-portal/signature-collection/src/screens/shared/SigneeView/index.tsx, and the import statementimport SigneeView from '../shared/SigneeView'in the Parliamentary screen component is correct. This import path is also consistently used in the Presidential screen component.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if 'SigneeView' exists in the shared directory. # Test: Look for 'SigneeView' files in the shared directory. fd --type f --name 'SigneeView.*' libs/service-portal/signature-collection/src/shared/Length of output: 339
Script:
#!/bin/bash # Description: Check for SigneeView files and references # Search for SigneeView files in the shared directory echo "Searching for SigneeView files:" fd SigneeView libs/service-portal/signature-collection/src/shared # Search for SigneeView references in the signature-collection directory echo "Searching for SigneeView references:" rg "SigneeView" libs/service-portal/signature-collectionLength of output: 1159
25-25: Ensurem.pageTitleParliamentaryis defined in messagesThe title prop now uses
formatMessage(m.pageTitleParliamentary). Please verify thatpageTitleParliamentaryis defined in your messages file and that it includes the necessary translations.Use the following script to check for the message definition:
Verification successful
pageTitleParliamentaryis correctly defined in messagesThe message
pageTitleParliamentaryis properly defined in the filelibs/service-portal/signature-collection/src/lib/messages.tson line 266. This confirms that the usage offormatMessage(m.pageTitleParliamentary)in the component is correct and should work as intended.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Confirm that 'pageTitleParliamentary' exists in the messages. # Test: Search for 'pageTitleParliamentary' in message files. fd --type f --name 'messages.ts' | xargs grep 'pageTitleParliamentary'Length of output: 319
Script:
#!/bin/bash # Description: Verify the existence of 'pageTitleParliamentary' in message files # Search for 'messages.ts' files and look for various forms of 'pageTitleParliamentary' fd -t f 'messages.ts' | xargs grep -n -E '(m\.)?pageTitleParliamentary(:|\s*=)' # If the above doesn't find anything, perform a broader search if [ $? -ne 0 ]; then echo "Broader search:" fd -t f -e ts -e js | xargs grep -n -E '(m\.)?pageTitleParliamentary' fiLength of output: 190
2-2: Verify the usage ofuseNamespacesfor localizationThe addition of
useNamespacessuggests that this component now relies on a specific localization namespace. Please ensure that the'sp.signatureCollection'namespace is correctly defined and includes all necessary translation strings.You can run the following script to confirm the existence of the namespace:
Verification successful
Verified: The 'sp.signatureCollection' namespace is correctly implemented and used
The addition of
useNamespaces('sp.signatureCollection')in the filelibs/service-portal/signature-collection/src/screens/Parliamentary/index.tsxis correct and consistent with the implementation throughout the signature-collection module. The namespace is properly defined in the messages file and used in multiple components within the module. This change ensures that the component has access to the necessary translations for the signature collection feature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the 'sp.signatureCollection' namespace exists in localization files. # Test: Search for definitions of 'sp.signatureCollection' in all JSON files. fd --type f --name '*.json' | xargs grep '"sp.signatureCollection"'Length of output: 320
Script:
#!/bin/bash # Description: Verify that the 'sp.signatureCollection' namespace exists in localization files. # Search for 'sp.signatureCollection' in JSON files echo "Searching for 'sp.signatureCollection' namespace:" find . -name '*.json' -type f -print0 | xargs -0 grep -H '"sp\.signatureCollection"' echo -e "\nSearching for 'sp' namespace (potential parent):" find . -name '*.json' -type f -print0 | xargs -0 grep -H '"sp":' echo -e "\nListing all JSON files in the repository:" find . -name '*.json' -type fLength of output: 135771
Script:
#!/bin/bash # Search for 'sp.signatureCollection' namespace in TypeScript and JavaScript files echo "Searching for 'sp.signatureCollection' namespace in TS/JS files:" find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" \) -print0 | xargs -0 grep -H "sp\.signatureCollection" echo -e "\nSearching for files that might define or use 'useNamespaces':" find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" \) -print0 | xargs -0 grep -H "useNamespaces" echo -e "\nListing files in the signature-collection directory:" find ./libs/service-portal/signature-collection -type fLength of output: 53637
libs/service-portal/signature-collection/src/screens/Presidential/OwnerView/index.tsx (2)
20-20: Import statement updated correctlyThe
SignedListcomponent import has been updated to reflect the new directory structure, which enhances modularity and promotes better code organization.
88-90: Proper use of theSignedListcomponent withcurrentCollectionpropThe
SignedListcomponent is correctly utilized with thecurrentCollectionprop, and the conditional rendering based on!user?.profile.actorensures it displays appropriately for users without an actor profile.libs/service-portal/signature-collection/src/screens/shared/SignedList/index.tsx (1)
Line range hint
45-53: Handle cases where multiple signed lists are presentIn the mutation variables for
unSign, thelistIdis set only when there is exactly one signed list. If there are multiple signed lists or none,listIdbecomesundefined, which may cause unintended behavior or errors during the mutation.Consider enhancing the logic to handle multiple signed lists appropriately. You might prompt the user to select which list they wish to un-sign or provide feedback if un-signing cannot proceed.
Please verify that the backend handles an
undefinedlistIdgracefully and that appropriate error messages are displayed to the user.libs/service-portal/signature-collection/src/lib/messages.ts (2)
19-19: Update topageDescriptionSigneedefault message looks goodThe new message provides clearer instructions to the user in Icelandic.
32-36: Addition ofcollectionTitleParliamentarymessage is appropriateThe new message supports the parliamentary elections feature and follows the existing naming conventions.
libs/service-portal/signature-collection/src/screens/shared/SigneeView/index.tsx (2)
46-46: PassingcurrentCollectiontoSignedListcomponentPassing the
currentCollectionprop to theSignedListcomponent enhances its contextual awareness and allows it to render information specific to the current signature collection.
63-66: Conditional rendering of text based on collection typeThe conditional rendering of the
textprop ensures that the correct message is displayed for presidential or parliamentary collections, providing users with contextually appropriate information.
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/index.tsx
Show resolved
Hide resolved
libs/service-portal/signature-collection/src/screens/shared/SignedList/index.tsx
Show resolved
Hide resolved
libs/service-portal/signature-collection/src/screens/shared/SignedList/index.tsx
Show resolved
Hide resolved
albinagu
added
the
deprecated:automerge
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
OwnerView, improving maintainability.Refactor
SignedListcomponent to accept acurrentCollectionprop for dynamic rendering based on collection type.Style
SkeletonLoaderfor a more compact layout.