feat(web): Add default header for GEV organization #16112
Conversation
WalkthroughThe changes introduce conditional rendering logic in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (2)
apps/web/components/Organization/Wrapper/OrganizationWrapper.tsx (2)
Line range hint
1-1176: Consider refactoring to improve maintainability and reduce repetitionWhile the changes for the GEV organization are correct, the overall structure of the
OrganizationHeadercomponent could be improved:
- The large switch statement could be replaced with a more scalable solution, such as a mapping object or a factory pattern.
- There's repetition in the logic for different organizations (e.g., checking
usingDefaultHeaderand renderingDefaultHeader).- Consider extracting the header logic for each organization into separate components or configuration objects.
These improvements would make the code more maintainable and easier to extend for future organizations.
Here's a high-level example of how you might refactor this:
const organizationHeaders = { syslumenn: { getHeader: (props) => n('usingDefaultHeader', false) ? <SyslumennDefaultHeader {...props} /> : <SyslumennHeader {...props} />, }, gev: { getHeader: (props) => n('usingDefaultHeader', false) ? <DefaultHeader {...props} image={n('gevHeaderImage', 'default-url')} /> : <GevHeader {...props} />, }, // ... other organizations }; const OrganizationHeader: React.FC<HeaderProps> = ({ organizationPage, isSubpage }) => { const headerConfig = organizationHeaders[organizationPage.theme] || {}; return headerConfig.getHeader({ organizationPage, isSubpage, logoAltText }) || <DefaultHeader {...defaultProps} />; };This approach would make it easier to add new organizations and maintain existing ones.
Line range hint
1-1176: Summary: GEV changes are good, but consider broader refactoringThe changes for the GEV organization header are implemented correctly and fulfill the PR objectives. The new default header option is well-integrated and maintains backward compatibility.
However, as the number of organization-specific headers grows, it might be beneficial to consider a broader refactoring of the
OrganizationHeadercomponent to improve maintainability and scalability. This could involve:
- Modularizing organization-specific logic
- Implementing a more flexible configuration system
- Reducing code duplication
These improvements could be addressed in a future PR to keep the current changes focused and minimize risk.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- apps/web/components/Organization/Wrapper/OrganizationWrapper.tsx (1 hunks)
Additional context used
Path-based instructions (1)
apps/web/components/Organization/Wrapper/OrganizationWrapper.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
Additional comments not posted (1)
apps/web/components/Organization/Wrapper/OrganizationWrapper.tsx (1)
424-432: LGTM! New default header option for GEV organization implemented correctly.The implementation looks good and aligns with the PR objectives. It introduces a configurable default header for the GEV organization while maintaining the existing custom header as a fallback. A few observations:
- The condition
n('usingDefaultHeader', false)allows for easy configuration.- The default header uses a custom image URL, which can be easily changed if needed.
- The existing
GevHeaderis kept as a fallback, ensuring backward compatibility.To ensure this change doesn't affect other parts of the codebase, let's verify the usage of
OrganizationHeader:Verification successful
Verified: No impact on existing usages of
OrganizationHeader.All existing instances of
OrganizationHeaderpass only theorganizationPageprop. Since the newusingDefaultHeaderprop defaults tofalse, current functionality remains unchanged.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of OrganizationHeader component rg --type typescript "OrganizationHeader" -A 5 -B 5Length of output: 86
Script:
#!/bin/bash # Search for usage of OrganizationHeader component in .ts and .tsx files rg "OrganizationHeader" --type-add 'typescript:*.ts,*.tsx' --type typescript -A 5 -B 5Length of output: 235
Script:
#!/bin/bash # Search for usage of OrganizationHeader component across all files rg "OrganizationHeader" -A 5 -B 5Length of output: 8221
Script:
#!/bin/bash # Extract props passed to OrganizationHeader component in all files rg "<OrganizationHeader\s+([^>]*)>" -A 5Length of output: 1812
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16112 +/- ##
==========================================
- Coverage 36.64% 36.64% -0.01%
==========================================
Files 6748 6748
Lines 138813 138815 +2
Branches 39450 39452 +2
==========================================
Hits 50863 50863
- Misses 87950 87952 +2
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 ReportBranch report: ✅ 0 Failed, 84 Passed, 0 Skipped, 29.68s Total Time |
Add default header for GEV organization
What
Make it possible to use default header for Gæða- og eftirlitsstofnun velferðarmála organization via config.
Why
A design that was approved by Digital Iceland
Screenshots / Gifs
Before
After
Checklist:
Summary by CodeRabbit