feat(web): Change default header styles for organizations #16218
Conversation
mannipje
added
the
deprecated:automerge
WalkthroughThe changes involve the addition of new CSS styles for various organization subpage layouts and modifications to the 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 (4)
apps/web/components/Organization/Wrapper/OrganizationWrapper.css.ts (3)
37-47: LGTM with a minor suggestion.The new
sakHeaderGridContainerSubpagestyle is well-structured and consistent with the existing patterns. It appropriately uses responsive styling and allows for flexible content height on subpages.Consider using a shared constant for the
maxWidthvalue (1342px) to improve maintainability, as this value is repeated across multiple styles in the file.
96-106: LGTM. Consider refactoring to reduce duplication.The
shhHeaderGridContainerWidthSubpagestyle is consistent with the previous subpage styles, maintaining the same structure and best practices.To reduce code duplication, consider creating a base subpage style that can be extended or composed for each specific organization. This would improve maintainability and reduce the likelihood of inconsistencies. For example:
const baseSubpageStyle = style({ display: 'grid', maxWidth: '1342px', margin: '0 auto', ...themeUtils.responsiveStyle({ lg: { gridTemplateRows: 'auto', gridTemplateColumns: '100fr', }, }), }) export const shhHeaderGridContainerWidthSubpage = style([ baseSubpageStyle, // Add any SHH-specific styles here ])
Line range hint
1-254: Overall assessment: Changes look good with room for improvement.The new subpage styles effectively address the PR objective of correcting header height issues on subpages. The implementation is consistent across all new styles and follows NextJS and TypeScript best practices.
Key points:
- All new styles maintain a consistent structure, improving maintainability.
- Organization-specific styling is handled appropriately where needed.
- The use of
themeUtils.responsiveStyleensures proper responsive behavior.To further improve the code:
- Consider creating a base subpage style to reduce duplication across the various organization-specific styles.
- Use a shared constant for common values like
maxWidth: '1342px'to improve maintainability.These refactoring suggestions will enhance code maintainability and reduce the likelihood of inconsistencies in future updates.
apps/web/components/Organization/Wrapper/OrganizationWrapper.tsx (1)
459-463: LGTM: Consistent styling approach, consider refactoringThe addition of a conditional className based on the
isSubpageprop for thehvecase is consistent with the previous cases and aligns with the PR's objective.Consider refactoring this repeated pattern across multiple organization cases to reduce code duplication. You could create a helper function that generates the appropriate className based on the theme and
isSubpageprop. For example:const getHeaderClassName = (theme: string, isSubpage: boolean) => { const themeMap = { landlaeknir: [styles.landlaeknirHeaderGridContainer, styles.landlaeknirHeaderGridContainerSubpage], sak: [styles.sakHeaderGridContainer, styles.sakHeaderGridContainerSubpage], hve: [styles.hveHeaderGridContainer, styles.hveHeaderGridContainerSubpage], // Add other themes here }; return isSubpage ? themeMap[theme][1] : themeMap[theme][0]; }; // Usage className={getHeaderClassName('hve', isSubpage)}This refactoring would make the code more maintainable and easier to extend for future theme additions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/web/components/Organization/Wrapper/OrganizationWrapper.css.ts (6 hunks)
- apps/web/components/Organization/Wrapper/OrganizationWrapper.tsx (3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/web/components/Organization/Wrapper/OrganizationWrapper.css.ts (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."
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 (7)
apps/web/components/Organization/Wrapper/OrganizationWrapper.css.ts (4)
61-71: LGTM. Consistent with previous styles.The
landlaeknirHeaderGridContainerSubpagestyle is consistent with thesakHeaderGridContainerSubpagestyle, following the same structure and best practices.
139-155: LGTM. Organization-specific styling handled well.The
rikissaksoknariHeaderGridContainerSubpagestyle maintains consistency with other subpage styles while incorporating organization-specific background styling. The use of template literals for complex background values is appropriate and improves readability.If you decide to implement the base subpage style as suggested earlier, ensure that this organization-specific background styling is preserved when extending the base style.
169-179: LGTM. Consistent with other subpage styles.The
hsaHeaderGridContainerWidthSubpagestyle maintains consistency with the previously reviewed subpage styles, following the same structure and best practices.This style further emphasizes the potential benefit of creating a base subpage style to reduce duplication, as suggested earlier. Consider implementing this refactoring to improve code maintainability.
243-254: LGTM. Organization-specific border styling handled appropriately.The
hveHeaderGridContainerSubpagestyle maintains consistency with other subpage styles while incorporating an organization-specific border styling. This approach allows for customization while keeping the overall structure consistent.When implementing the suggested base subpage style, ensure that this organization-specific border styling is preserved. You might consider using a composition approach to combine the base style with organization-specific styles:
export const hveHeaderGridContainerSubpage = style([ baseSubpageStyle, { borderBottom: '8px solid #F01E28', } ])This approach would maintain consistency while allowing for organization-specific customizations.
apps/web/components/Organization/Wrapper/OrganizationWrapper.tsx (3)
358-362: LGTM: Conditional styling for subpagesThe addition of conditional className based on the
isSubpageprop is a good approach to apply different styles for subpages and main pages. This change aligns well with the PR objective of correcting header height issues on subpages.
423-427: LGTM: Consistent subpage styling approachThe addition of a conditional className based on the
isSubpageprop for thesakcase is consistent with the previouslandlaeknircase. This change maintains a uniform approach to applying different styles for subpages and main pages across different organization themes.
Line range hint
1-1391: Summary: Consistent implementation of subpage styling across themesThe changes in this file focus on applying different styles for subpages across various organization themes (
landlaeknir,sak,hve). The implementation is consistent and aligns well with the PR objective of correcting header height issues on subpages.While the changes are correct and achieve the desired outcome, there's an opportunity to refactor the repeated pattern to reduce code duplication and improve maintainability. Consider implementing a helper function as suggested in the previous comment.
Overall, these changes provide better control over header styling for different page types without introducing new functionality.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16218 +/- ##
==========================================
- Coverage 36.70% 36.69% -0.01%
==========================================
Files 6781 6779 -2
Lines 139720 139691 -29
Branches 39725 39731 +6
==========================================
- Hits 51284 51265 -19
+ Misses 88436 88426 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 84 Passed, 0 Skipped, 31.18s Total Time |
kodiakhq
bot
deleted the
feature/change-default-header-styles-for-organizations
branch
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Change default header styles for organizations
What
CSS was wrong, it shouldn't be setting the header height to 315 px on subpages
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes