BA-2345-change-link-navigation-from-navbar #227
Conversation
marcelopessini
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThis change updates various navigation components’ link styling and related declarations. The Changes
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/components/modules/navigations/web/NavMini/NavSectionMini/NavList/NavItem/index.tsx (1)
88-94: Same styling concerns as external linkThe same concerns about
width: 1and thecolor="inherit"prop apply here. Consider usingwidth: '100%'for clarity and either removing the color prop or moving the color styling to the style object.<Link href={path} color="inherit" style={{ - width: 1, + width: '100%', + color: 'inherit', ...(disabled && { pointerEvents: 'none', }), }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/components/modules/navigations/web/NavMini/NavSectionMini/NavList/NavItem/index.tsx(3 hunks)packages/components/modules/navigations/web/__shared__/NavSectionHorizontal/NavList/NavItem/styled.tsx(1 hunks)packages/components/modules/navigations/web/__shared__/NavSectionVertical/Group/NavList/NavItem/index.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
packages/components/modules/navigations/web/__shared__/NavSectionHorizontal/NavList/NavItem/styled.tsx (2)
1-3: Successfully migrated from MUI Link to Next.js LinkThe change from Material-UI's Link to Next.js's Link component aligns with the PR objective and will provide better navigation capabilities, including client-side navigation without full page refreshes, automatic code-splitting, and prefetching.
124-135: Verify that styling is properly applied to Next.js LinkThe StyledLink component now applies Material-UI styling to Next.js's Link component instead of MUI's Link. While this should work as expected, the APIs are slightly different between the two components. Please verify that all styling properties in the StyledLink component (including the conditional styling) work correctly with Next.js Link.
packages/components/modules/navigations/web/__shared__/NavSectionVertical/Group/NavList/NavItem/index.tsx (2)
5-6: Successfully migrated from MUI Link to Next.js LinkThe change from Material-UI's Link to Next.js's Link component aligns with the PR objective and provides improved navigation capabilities.
92-97: Successfully implemented disabled state handlingThe inline style approach for handling the disabled state is correctly implemented, setting pointerEvents to 'none' when the link is disabled.
packages/components/modules/navigations/web/NavMini/NavSectionMini/NavList/NavItem/index.tsx (2)
8-8: Successfully migrated from MUI Link to Next.js LinkThe change from Material-UI's Link to Next.js's Link component aligns with the PR objective.
73-79:❓ Verification inconclusive
Review the styling approach for width and color
Two potential issues to consider:
The
width: 1in an inline style might not work as expected. In MUI's sx prop system, this would mean 100%, but in standard CSS it would be 1px. Consider usingwidth: '100%'instead.Next.js Link component doesn't natively support the
colorprop (line 72) like MUI's Link does. This prop might not have any effect.
🏁 Script executed:
#!/bin/bash # Check how width:1 is used in inline styles elsewhere in the codebase rg "style=\{.*width:\s*1" --type tsxLength of output: 66
Below are new verification commands to re-run the searches using file globs (instead of the unrecognized “tsx” file type) so we can reliably check for the issues raised:
Once you run these commands and review the outputs, please verify the following:
Inline Style Calculation:
If “width: 1” is used elsewhere and is intended to be interpreted as pixels, this inline style is standard CSS (i.e. 1px). However, if a component is expected to use MUI’s sx prop (where numeric values might be interpreted differently), you may need to adjust it—possibly to an explicit string (e.g.,"1px") or to a value like"100%"if that was the intended behavior.Link Component Props:
Check if the Next.js Link component (as imported fromnext/link) is being passed acolorprop. Since Next.js Link does not forward arbitrary styling props (unlike MUI’s Link), thecolorprop may have no effect.Please re-run the commands above and verify whether these patterns exist in the codebase. Based on what you find, adjust the inline style and/or remove or adapt the
colorprop for the Next.js Link component.
🏁 Script executed:
[run_scripts] #!/bin/bash # Verify usage of inline style "width: 1" in TSX files rg "style=\{\s*width:\s*1" -g "*.tsx"Length of output: 161
🏁 Script executed:
[run_scripts] #!/bin/bash # Verify usage of Next.js Link component with the color prop in TSX files rg "<Link[^>]*color=" -g "*.tsx"Length of output: 156
Action: Review Inline Style and Link Component Usage
Inline Style Width:
Inpackages/components/modules/navigations/web/NavMini/NavSectionMini/NavList/NavItem/index.tsx(lines 73–79), the inline style is currently usingwidth: 1. In standard CSS, this is interpreted as 1px. If the intent was to mimic MUI’s sx prop behavior (where numeric values can denote percentages or spacing), consider switching to a string value (e.g.,"1px"for a one-pixel width or"100%"if full-width is intended).Next.js Link Component:
The code appears to pass acolorprop to the Next.jsLinkcomponent. However, Next.js’sLinkdoes not natively support thecolorprop like MUI’s Link component does. If you need to apply a text color, consider styling the element inside the Link or wrapping it with a component that accepts a color prop.Please verify these points manually to ensure the chosen styling approach aligns with your intended design and functionality.
| <Link | ||
| href={path} | ||
| color="inherit" | ||
| underline="none" | ||
| sx={{ | ||
| style={{ | ||
| ...(disabled && { | ||
| pointerEvents: 'none', | ||
| }), | ||
| }} | ||
| href={path} | ||
| color="inherit" | ||
| > |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Review the color="inherit" prop on Next.js Link
Next.js Link component doesn't natively support the color prop like MUI's Link does. This prop might not have any effect when passed to Next.js Link. Consider either:
- Removing the color prop as it won't be used by Next.js Link
- Moving the color styling to the style object or to the child component
🏁 Script executed:
#!/bin/bash
# Check if the color prop is being used elsewhere in the codebase with Next.js Link
rg -A 5 "Link.*color=\"inherit\"" --type tsxLength of output: 72
Action Required: Verify Usage of the color="inherit" Prop on Next.js Link
It appears that the initial search failed due to an unrecognized file type for TSX files. To ensure we’re accurately identifying any usage of the color="inherit" prop on Next.js Link components, please run the revised shell script below:
This will help confirm if the prop is inadvertently being relied upon anywhere in the codebase. Once verified, consider removing the prop or moving its styling to where it can be applied effectively.
🏁 Script executed:
#!/bin/bash
# Revised search: Scan all .tsx files for Next.js Link usage with the `color="inherit"` prop.
rg -g '*.tsx' -A 5 'Link.*color="inherit"'Length of output: 44
Action Required: Adjust Usage of the color="inherit" Prop on Next.js Link
The code in
packages/components/modules/navigations/web/__shared__/NavSectionVertical/Group/NavList/NavItem/index.tsx (lines 103–111) is passing a color="inherit" prop to a Next.js Link component—even though Next.js’s Link does not natively support this prop. Our search for additional instances of this usage produced no further evidence that the prop is being utilized as intended elsewhere in the codebase.
Please address this by either:
- Removing the Prop: Simply eliminate
color="inherit"since it has no effect on Next.js Link. - Relocating the Styling: If inheriting a color is desired, apply the styling via the inline style object or wrap the Link’s child element (typically an
<a>tag) with the appropriate styling.
Make sure to update any related styling or tests accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
packages/design-system/layouts/native/BottomNavigationLayout/types.ts (1)
16-16: Good addition for more flexible navigation.The
routesWithoutHeaderoptional prop enhances the component by allowing developers to specify which routes should not display a header. This improves the flexibility of the BottomNavigationLayout.Consider adding JSDoc comments to document the purpose of this property, explaining when and how it should be used for better developer experience.
packages/design-system/layouts/native/AppbarNavigationLayout/types.ts (1)
1-5: Clean interface definition for navigation props.The interface is well-structured with appropriate types for navigation properties. This provides a clear contract for components utilizing the AppbarNavigationLayout.
Consider adding JSDoc comments to describe the purpose of each property, especially the callback functions, to improve code documentation:
+/** + * Props for the AppbarNavigationLayout component + */ export interface AppbarNavigationLayoutProps { + /** Title to display in the appbar */ title: string + /** Optional callback function when back button is pressed */ onBack?: () => void + /** Optional callback function when close button is pressed */ onClose?: () => void }packages/design-system/components/native/views/View/index.tsx (1)
9-9: Changed default keyboard dismissal behavior.Changing the default value of
dismissKeyboardfromtruetofalseis a significant behavior change. This means Views will no longer dismiss the keyboard by default when pressed.Please ensure all existing usages of this component have been reviewed and updated as needed to maintain expected behavior. Since this is a base component, this change could have widespread effects.
Consider adding a comment explaining the rationale behind this change to help future maintainers understand the decision:
-const View: FC<ViewProps> = ({ style, dismissKeyboard = false, ...props }) => { +/** + * View component with configurable keyboard dismissal behavior + * @note Default dismissKeyboard behavior changed from true to false in v1.x.x to provide more + * granular control over keyboard interactions + */ +const View: FC<ViewProps> = ({ style, dismissKeyboard = false, ...props }) => {packages/design-system/components/native/icons/BlockIcon/index.tsx (1)
1-38: Solid implementation of the new BlockIcon component.The implementation follows the established pattern for SVG icons in the design system with proper typing, theme integration, and SVG rendering. The component correctly handles the active state and fallback colors.
A few minor suggestions:
- Consider using number types for width and height props instead of strings for better type safety
- You might want to add JSDoc comments to document the component's purpose and prop usage
- width = '20', - height = '21', + width = 20, + height = 21,And in the Svg component:
- <Svg width={width} height={height} viewBox="0 0 20 21" color={svgColor} fill="none" {...props}> + <Svg width={`${width}`} height={`${height}`} viewBox="0 0 20 21" color={svgColor} fill="none" {...props}>packages/design-system/components/native/icons/SecurityIcon/index.tsx (1)
1-49: Well-structured SecurityIcon implementation.The component follows the same pattern as other icon components in the design system, with proper theme integration and SVG rendering. The code is clean and well-organized.
Similar to the BlockIcon, consider standardizing the width and height props to number types rather than strings for better type consistency across the component library.
- width = '20', - height = '21', + width = 20, + height = 21,And update the Svg component accordingly:
- <Svg width={width} height={height} viewBox="0 0 20 21" color={svgColor} fill="none" {...props}> + <Svg width={`${width}`} height={`${height}`} viewBox="0 0 20 21" color={svgColor} fill="none" {...props}>packages/design-system/components/native/icons/CloseIcon/index.tsx (1)
1-32: CloseIcon implementation is consistent with other icon components.The component follows the established pattern for icon components with proper theming and SVG rendering. The implementation is clean and well-structured.
I notice that this icon uses slightly different dimensions ('24' and '25') compared to the other icons ('20' and '21'). This difference is fine if intentional, but ensure it aligns with your design system's specifications for icon sizes.
As with the other icons, consider standardizing to number types for width and height.
- width = '24', - height = '25', + width = 24, + height = 25,And update the Svg component accordingly:
- <Svg width={width} height={height} viewBox="0 0 24 25" color={svgColor} {...props}> + <Svg width={`${width}`} height={`${height}`} viewBox="0 0 24 25" color={svgColor} {...props}>packages/design-system/components/native/avatars/ClickableAvatar/styles.ts (1)
1-29: Well-organized styles for the ClickableAvatar component.The styles are properly structured with a clear separation of container, image, and fallback states. The use of the theme system for colors is consistent with the design system's approach.
One observation regarding the container sizing: you're adding 4 pixels to both width and height (width + 4, height + 4) to accommodate the border. While this works, consider if it might be clearer to use calculations that explicitly show the border's contribution:
- height: height + 4, - width: width + 4, + height: height + (2 * 2), // height + (borderWidth * 2) + width: width + (2 * 2), // width + (borderWidth * 2)This makes it more obvious that the extra space is specifically for the 2px border on each side.
packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx (1)
248-341: Improved component structureThe restructuring of the component layout improves hierarchy and organization. Moving from nested Views to a more streamlined structure with clearer container responsibilities makes the code more maintainable.
While the changes look good overall, consider extracting some of these nested components into separate components to further improve readability and maintainability, especially for complex UI sections like the image handling components.
packages/design-system/components/native/avatars/ClickableAvatar/index.tsx (1)
1-37: Well-implemented new ClickableAvatar component with good separation of concerns.The component follows React best practices with a clean functional implementation. I appreciate:
- The use of default props for height, width, and fallbackMode
- Proper separation of styling logic
- Conditional rendering based on fallback state
- Clean component structure with appropriate props handling
One small enhancement would be to add accessibility attributes to the Pressable component, such as
accessibilityRole="button"and potentially anaccessibilityLabelbased on context.- <Pressable onPress={onPress}> + <Pressable + onPress={onPress} + accessibilityRole="button" + accessibilityLabel="Avatar">packages/design-system/components/native/appbars/AppBar/index.tsx (2)
11-17: Ensure safe defaults & consider optional props
Providing default icons (BackIconandCloseIcon) is a nice approach. However, iftitleis not specified, you may want to default it to an empty string or visually hide it to avoid layout inconsistencies.
22-40: Optimize container usage
You have two separate containers for back and close buttons, which may result in extra spacing or layout complexity when the actions are not provided. Consider rendering these button containers conditionally or merging them if it helps simplify styling.packages/components/package.json (2)
69-101: Pinned dependencies may restrict compatibility
Pinning your dependencies to exact versions (e.g.,"@emotion/cache": "11.11.0") can lead to version conflicts or missed critical patch updates. Consider using semver ranges (e.g.,^11.11.0) if there's no pressing need to lock them down.
112-177: Pinned devDependencies
Similar to your main dependencies, pinning devDependencies to exact versions can limit your ability to receive updates or security patches automatically. Ensure this aligns with your project’s maintenance and update policies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
packages/authentication/CHANGELOG.md(1 hunks)packages/authentication/modules/access/useSignUp/index.ts(1 hunks)packages/authentication/modules/user/useUpdateUser/index.ts(0 hunks)packages/authentication/package.json(1 hunks)packages/components/.gitignore(1 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/baseapp-frontend-imports.d.ts(1 hunks)packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx(1 hunks)packages/components/modules/profiles/native/ProfileSettingsComponent/styles.ts(0 hunks)packages/components/package.json(2 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/native/appbars/AppBar/index.tsx(1 hunks)packages/design-system/components/native/appbars/AppBar/styles.ts(1 hunks)packages/design-system/components/native/appbars/AppBar/types.ts(1 hunks)packages/design-system/components/native/avatars/ClickableAvatar/index.tsx(1 hunks)packages/design-system/components/native/avatars/ClickableAvatar/styles.ts(1 hunks)packages/design-system/components/native/avatars/ClickableAvatar/types.ts(1 hunks)packages/design-system/components/native/avatars/index.ts(1 hunks)packages/design-system/components/native/icons/BellIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/BlockIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/ChevronIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/ChevronIcon/types.ts(1 hunks)packages/design-system/components/native/icons/CloseIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/LogoutIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/ProfileSettingsIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/SecurityIcon/index.tsx(1 hunks)packages/design-system/components/native/icons/index.ts(1 hunks)packages/design-system/components/native/views/PageViewWithHeader/index.tsx(1 hunks)packages/design-system/components/native/views/View/index.tsx(1 hunks)packages/design-system/layouts/native/AppbarNavigationLayout/index.tsx(1 hunks)packages/design-system/layouts/native/AppbarNavigationLayout/styles.ts(1 hunks)packages/design-system/layouts/native/AppbarNavigationLayout/types.ts(1 hunks)packages/design-system/layouts/native/BottomNavigationLayout/index.tsx(1 hunks)packages/design-system/layouts/native/BottomNavigationLayout/types.ts(1 hunks)packages/design-system/layouts/native/index.ts(1 hunks)packages/design-system/package.json(2 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
💤 Files with no reviewable changes (2)
- packages/authentication/modules/user/useUpdateUser/index.ts
- packages/components/modules/profiles/native/ProfileSettingsComponent/styles.ts
✅ Files skipped from review due to trivial changes (5)
- packages/authentication/package.json
- packages/design-system/layouts/native/AppbarNavigationLayout/styles.ts
- packages/wagtail/package.json
- packages/design-system/components/native/avatars/index.ts
- packages/authentication/CHANGELOG.md
🧰 Additional context used
🪛 LanguageTool
packages/components/CHANGELOG.md
[style] ~7-~7: Consider a shorter alternative to avoid wordiness.
Context: ...changed from MUI Link to Next Link, in order to prevent cache loss between app routes. ...
(IN_ORDER_TO_PREMIUM)
🔇 Additional comments (29)
packages/components/.gitignore (1)
64-66: Ensure generated/.keep is Tracked as IntendedThe new line
!/__generated__/.keepcorrectly unignores the.keepfile within the__generated__directory. This ensures that the file is tracked by Git—helpful for keeping the directory structure intact—even while the rest of the directory remains ignored.packages/authentication/modules/access/useSignUp/index.ts (1)
42-48: Error handling improvement looks good.This change appropriately removes a likely
console.error(error)statement and replaces it with a clarifying comment. Since errors are already handled by the mutation'sonErrorcallback (defined on lines 31-36), this prevents duplicate error handling and follows the DRY principle. TheonErrorcallback already provides comprehensive error handling by calling any user-provided handler and setting form errors when enabled.packages/design-system/components/native/views/PageViewWithHeader/index.tsx (1)
8-8: Explicit keyboard dismissal behavior is now correctly specified.This change properly updates the component to explicitly enable keyboard dismissal, which is necessary since the default behavior in the View component has been changed to
false(as seen in the View component file).packages/wagtail/CHANGELOG.md (1)
3-8: LGTM: Version and dependency updates are properly documented.The changelog correctly documents the patch version increment to 1.0.23 and updates the design-system dependency to version 1.0.6.
packages/components/CHANGELOG.md (3)
3-8: LGTM: New version documents important change to navigation links.The changelog appropriately uses a minor version bump (1.1.0) for the functional change of switching from MUI Link to Next Link components, with a clear explanation of the benefit (preventing cache loss between app routes).
🧰 Tools
🪛 LanguageTool
[style] ~7-~7: Consider a shorter alternative to avoid wordiness.
Context: ...changed fromMUI LinktoNext Link, in order to prevent cache loss between app routes. ...(IN_ORDER_TO_PREMIUM)
9-15: LGTM: Dependency update properly documented.The changelog correctly documents the patch version increment to 1.0.14 with updated dependencies.
16-22: LGTM: Component changes and dependency updates properly documented.The changelog correctly documents the removal of
ProfileSettingsComponentAppbar in favor of usingAppbarNavigationLayout, along with dependency updates.packages/components/baseapp-frontend-imports.d.ts (1)
16-16: LGTM: Added import for native avatars components.This addition properly extends the TypeScript auto-import suggestions to include the avatars component from the design system.
packages/design-system/layouts/native/AppbarNavigationLayout/index.tsx (1)
1-17: LGTM: Clean implementation of the AppbarNavigationLayout component.This new component follows React best practices using functional components with proper TypeScript typing. It correctly implements a navigation layout that includes an AppBar component and supports nested routing via expo-router's Slot component.
packages/design-system/components/native/icons/index.ts (1)
1-2: New icon exports added successfullyThe new icons (BellIcon, BlockIcon, ChevronIcon, CloseIcon, LogoutIcon, ProfileSettingsIcon, SecurityIcon) have been successfully added to the icon exports. This follows the existing pattern consistently and maintains alphabetical ordering.
Also applies to: 5-6, 11-11, 16-17
packages/design-system/layouts/native/index.ts (1)
3-5: AppbarNavigationLayout export added correctlyThe new AppbarNavigationLayout component and its types have been correctly exported, maintaining consistency with the existing pattern used for other layout components.
packages/design-system/components/native/icons/ProfileSettingsIcon/index.tsx (1)
1-45: New ProfileSettingsIcon implementation looks goodThe implementation follows the design system's icon pattern with proper theme integration, prop handling, and SVG structure.
Two minor observations:
- The component uses the useTheme hook to access colors, consistent with other icons
- The color handling logic correctly prioritizes the active state color and falls back to the default color
packages/design-system/components/native/icons/LogoutIcon/index.tsx (1)
1-47: LogoutIcon implementation follows design patterns correctlyThe LogoutIcon is well-implemented, following the same pattern as other icon components in the design system with consistent prop handling, theme usage, and SVG structure.
The SVG paths appropriately represent a logout action with clean stroke styling.
packages/design-system/components/native/icons/BellIcon/index.tsx (1)
1-40: Well-structured icon component implementationThe BellIcon component is well implemented with clear prop definitions, proper theme integration, and responsive color handling based on active state. The SVG paths are correctly defined with appropriate stroke properties.
packages/design-system/package.json (2)
4-4: Version bump is appropriate for the changesThe version update from 1.0.5 to 1.0.6 follows semantic versioning for minor changes.
83-86: Dependency updates align with catalog-based managementThe addition of react-native-gesture-handler and updating react-native-svg to use catalog references aligns with the project's transition to catalog-based dependency management.
packages/design-system/components/native/appbars/AppBar/styles.ts (3)
7-15: Enhanced AppBar container stylingThe container style changes improve layout flexibility with better alignment and spacing properties. Removing hardcoded visual properties in favor of layout-focused ones allows for better theme integration.
16-19: Simplified title stylingThe title style has been appropriately simplified to focus on layout rather than specific visual properties, which aligns with a more theme-driven approach.
20-23: Good addition of button container stylingThe new buttonContainer style provides consistent alignment and sizing for buttons within the AppBar, enhancing the component's flexibility.
packages/design-system/components/native/icons/ChevronIcon/types.ts (1)
1-9: Type definitions look well structured for the new ChevronIcon component.The type definitions provide a clear and type-safe way to handle different directional states for the ChevronIcon. The
Directionstype accurately constrains the allowed values, and theRotateRecordtype creates a mapping that will likely be used for rotation transformations. The component props correctly extend the baseSvgIconPropswhile adding the optional direction property.packages/design-system/CHANGELOG.md (1)
3-12: Changelog entries accurately document the recent changes.The changelog appropriately captures all the significant updates to the design system package, including the redesigned Appbar component, new icons, and layout changes. The version increment follows semantic versioning practices correctly with these being classified as patch changes.
packages/design-system/components/native/appbars/AppBar/types.ts (1)
1-11: Type definitions appropriately updated for the redesigned AppBar component.The changes to the AppBar props reflect a more flexible and component-based approach by:
- Replacing the generic
goBackfunction with more specificonBackandonClosehandlers- Adding support for customizable icon components via
BackIconandCloseIconprops- Properly typing the icon components as functional components that accept SVG icon props
This improved type structure aligns well with the changelog entry about redesigning the Appbar component for better reusability.
packages/design-system/components/native/icons/ChevronIcon/index.tsx (1)
1-46: Well-structured and reusable ChevronIcon component!The implementation is clean and follows best practices with:
- Good use of FC type with proper TypeScript props
- Appropriate default values for key properties
- Theme integration for consistent styling across the app
- Smart direction handling with the rotation mapping
- Proper SVG implementation with viewBox and Path attributes
packages/design-system/components/native/avatars/ClickableAvatar/types.ts (1)
1-22: Excellent TypeScript type definitions with mutual exclusivity!The type system implemented here is very well designed:
- Good use of union and intersection types
- Proper implementation of mutually exclusive props with
nevertype- Clear naming conventions that reflect the component's purpose
- Types that enforce correct usage patterns at compile time
This is a great example of leveraging TypeScript to prevent runtime errors by catching invalid prop combinations during development.
packages/design-system/layouts/native/BottomNavigationLayout/index.tsx (2)
11-24: Great enhancement to navigation flexibility!The addition of
routesWithoutHeaderprop and related logic significantly improves the component's flexibility. The implementation:
- Properly handles conditional header visibility
- Uses defensive programming with optional chaining
- Correctly sets up the useLayoutEffect dependencies
- Maintains clean separation of UI and navigation logic
27-27: Good update to tab navigation behavior.Setting
backBehaviorto 'history' improves the user experience by making the back navigation more intuitive based on navigation history rather than tab order.packages/design-system/components/native/appbars/AppBar/index.tsx (1)
4-7: Imports look properly organized
The newly introduced imports forIconButton,DefaultBackIcon,DefaultCloseIcon,Text, andVieware consistent with the updated component design.packages/components/package.json (2)
4-4: Confirm semantic versioning alignment
Bumping from 1.0.12 to 1.1.0 typically indicates the addition of new features in a backwards-compatible manner. Ensure no breaking changes were introduced; otherwise, you may need a major version bump.
107-108: Check peer dependency ranges for React
Pinning React and ReactDOM to18.3.1might cause unnecessary restrictions for downstream consumers. Evaluate whether using a more flexible range (e.g.^18.3.0) is feasible.
pnpm-workspace.yaml
Outdated
| expo-constants: ~17.0.7 | ||
| expo-file-system: ~18.0.11 | ||
| expo-image-picker: ~16.0.4 | ||
| expo-router: ~4.0.5 | ||
| expo-secure-store: ~14.0.0 | ||
| react-native-gesture-handler: ~2.20.2 | ||
| react-native-paper: ^5.12.5 | ||
| react-native-reanimated: ~3.16.3 | ||
| react-native: 0.76.3 | ||
| react-native-gesture-handler: ~2.20.2 | ||
| react-native-svg: 15.8.0 | ||
| react-native: 0.76.7 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Dependency updates look good.
The updated dependencies and new react-native-svg addition align with the new ChevronIcon component requirements. These version upgrades appear to be appropriate maintenance updates.
🏁 Script executed:
#!/bin/bash
# Check if these are the latest safe versions of the updated packages
# Get the latest versions from npm registry
echo "Checking latest versions of updated packages:"
echo "expo-constants current: ~17.0.7"
npm view expo-constants version
echo "expo-file-system current: ~18.0.11"
npm view expo-file-system version
echo "react-native current: 0.76.7"
npm view react-native version
echo "react-native-svg current: 15.8.0"
npm view react-native-svg version
# Check for any security advisories
echo "Checking for security advisories:"
npm audit --json | grep -i "react-native\|expo-constants\|expo-file-system\|react-native-svg" || echo "No specific advisories found for these packages"Length of output: 1189
Action Required: Verify React Native and React Native SVG Versions
Most dependencies have been updated appropriately for the new ChevronIcon component requirements. However, please note the following discrepancies compared to the latest versions on npm:
- react-native: Currently pinned to 0.76.7 but the latest version is 0.78.0.
- react-native-svg: Currently set to 15.8.0 while 15.11.2 is available.
Please confirm whether these version selections were intentional (e.g., to maintain compatibility) or if updating to the latest safe versions is advisable.
marcelopessini
force-pushed
the
change-link-navigation-from-navbar
branch
5 times, most recently
from
ed0b1f6 to
d1a937b
Compare
marcelopessini
force-pushed
the
change-link-navigation-from-navbar
branch
from
d1a937b to
77df8ec
Compare
packages/components/modules/navigations/web/NavMini/__tests__/NavMini.cy.tsx
Outdated
Show resolved
Hide resolved
marcelopessini
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/components/package.json (2)
81-101: Adoption of Catalog References for Extended DependenciesMany additional packages (including Expo-related modules, GraphQL packages, Next.js, and various React Native dependencies) have been refactored to use catalog references. This streamlined approach to version management is beneficial. As a nitpick, you might consider adding documentation or a README note for maintainers on how to verify or update the catalog mapping.
113-174: Streamlined DevDependencies with Catalog ReferencesDevDependencies have been uniformly updated to reference catalog entries (e.g.,
"catalog:storybook","catalog:test", etc.) instead of fixed versions. This change should help maintain consistency across your tooling. Please double-check that these revisions are compatible with any local overrides or project-specific testing configurations.packages/design-system/package.json (2)
59-89: Reverting to Specific Dependency VersionsIn this package, dependencies (such as
"@emotion/cache","@mui/icons-material","expo-router", etc.) have been reverted from catalog references back to explicit version numbers. This reversion might be driven by the need for stability or compatibility within the design system. Please confirm that these fixed versions are compatible with your broader project ecosystem.
96-130: Restored DevDependencies VersioningDevDependencies have been reverted to specific version numbers (for packages such as
@babel/preset-env, Storybook addons, Tailwind, etc.), which can provide better stability and predictability during development. Double-check that these versions meet your project’s compatibility and security requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/components/package.json(1 hunks)packages/components/package.json(2 hunks)packages/design-system/package.json(2 hunks)packages/components/package.json(1 hunks)packages/design-system/package.json(1 hunks)packages/components/package.json(1 hunks)packages/design-system/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/design-system/package.json
- packages/components/package.json
- packages/design-system/package.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (4)
packages/components/package.json (2)
70-80: Consistent Catalog References for MUI Components and Related DependenciesThe dependencies for MUI packages (such as
@emotion/cache,@emotion/react,@emotion/styled, and others) have been updated to use catalog references (e.g.,"catalog:material-ui"). This enhances consistency across dependency management. Please ensure that your catalog service resolves these references to the intended versions.
108-109: Updated Peer Dependencies for React ConsistencyThe peer dependencies for
"react"and"react-dom"have been updated to"catalog:react18". Verify that downstream consumers and other packages are also aligned with this reference.packages/design-system/package.json (2)
17-18: Addition of the 'prepare' ScriptThe new
"prepare": "pnpm build"script complements the existing"prepublishOnly": "pnpm run build"command, ensuring that a build is executed before publishing. This is a helpful addition for automating prepublish steps.
92-93: Reverted Peer DependenciesThe peer dependencies for
"react"and"react-dom"have been set back to"18.3.1". Ensure that this aligns with the expectations of consumers of the Design System package.
… change-link-navigation-from-navbar
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/CHANGELOG.md (1)
7-7: Link Component Update DescriptionThe changelog now specifies the update to use Next.js’ Link for the
NavItemcomponent instead of Material UI’s Link. For improved readability, consider rephrasing to include the article “the” (e.g., “...to use the Link from next/link…”).🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ... ## 1.1.0 ### Minor Changes - ChangeLinkcomponent inNavItem, to use Lin...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/components/CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/components/CHANGELOG.md
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ... ## 1.1.0 ### Minor Changes - Change Link component in NavItem, to use Lin...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (2)
packages/components/CHANGELOG.md (2)
3-6: New Version Header and Section IntroductionThe new version header (
## 1.1.0) and the accompanying "Minor Changes" section have been added correctly. This clearly indicates the changes in the new release. Ensure that the version bump aligns with your semantic versioning guidelines.
8-8: Header Border Style UpdateThe documentation for changing the Header border bottom style from dashed to solid is clear and concise.
… change-link-navigation-from-navbar
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/CHANGELOG.md (1)
7-7: Stylistic Improvement for ClarityThe description in line 7 can be slightly rephrased for improved clarity and readability. For example, consider removing the comma and adding the article “the” to read as follows:
- - Change `Link` component in `NavItem`, to use Link from next/link instead of Material UI Link. + - Change the `Link` component in `NavItem` to use Next.js `Link` instead of Material UI `Link`.This minor edit will enhance the consistency and clarity of the changelog entry.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ... ## 1.1.0 ### Minor Changes - ChangeLinkcomponent inNavItem, to use Lin...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/components/CHANGELOG.md
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ... ## 1.1.0 ### Minor Changes - Change Link component in NavItem, to use Lin...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Analyze (javascript)
- GitHub Check: Unit Test Packages
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (1)
packages/components/CHANGELOG.md (1)
3-9: New Version Entry for 1.1.0 – Clear and Concise UpdatesThis new changelog entry correctly documents the minor changes in version 1.1.0. It clearly specifies the updates: switching the
Linkcomponent inNavItemto use Next.js’Linkand updating theHeaderborder style from dashed to solid. Overall, the format is consistent with previous entries.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ... ## 1.1.0 ### Minor Changes - ChangeLinkcomponent inNavItem, to use Lin...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
… change-link-navigation-from-navbar
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/CHANGELOG.md (1)
7-8: Clarify and Correct Changelog Description
For the bullet regarding the navigation link: It currently reads
- Change \Link` component in `NavItem`, to use Link from next/link instead of Material UI Link.`
Consider a minor grammatical improvement by adding the article "the". For example:- Change `Link` component in `NavItem`, to use Link from next/link instead of Material UI Link. + Change the `Link` component in `NavItem` to use Link from next/link instead of Material UI Link.This subtle change enhances clarity and grammatical correctness.
For the bullet regarding the header, the note
- Change Header border bottom from dashed to solidsuccinctly conveys the update.Overall, the descriptions are clear and match the PR objectives.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ... ## 1.1.0 ### Minor Changes - ChangeLinkcomponent inNavItem, to use Lin...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/components/CHANGELOG.md
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ... ## 1.1.0 ### Minor Changes - Change Link component in NavItem, to use Lin...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (2)
packages/components/CHANGELOG.md (2)
3-4: Version Update ConfirmationThe new version entry "## 1.1.0" correctly reflects a minor change update. This aligns with semantic versioning for the documented modifications.
5-6: Minor Changes Section ValidationThe heading "### Minor Changes" properly categorizes the changes in this release.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/CHANGELOG.md (1)
7-7: Improve Changelog Description Clarity:
The description for theLinkcomponent update reads:
- Change \Link` component in `NavItem`, to use Link from next/link instead of Material UI Link.Consider revising this to include the article “the” for enhanced clarity. For example:- Change the `Link` component in `NavItem` to use the Link from next/link instead of Material UI Link.`🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ... ## 1.0.21 ### Patch Changes - ChangeLinkcomponent inNavItem, to use Lin...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/package.json
🧰 Additional context used
🪛 LanguageTool
packages/components/CHANGELOG.md
[uncategorized] ~7-~7: You might be missing the article “the” here.
Context: ... ## 1.0.21 ### Patch Changes - Change Link component in NavItem, to use Lin...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (2)
packages/components/CHANGELOG.md (2)
3-3: Version Entry Addition Check:
The new version entry "## 1.0.21" is added correctly. Please ensure that the version bump follows semver guidelines and accurately reflects the patch nature of the changes.
8-8: Header Style Update Noted:
The change indicating a border bottom style update from dashed to solid for the Header is clearly documented. Please confirm that this visual change has been validated across relevant browsers and layout contexts.
Change navbar Link to use next/Link intead of MUI Link
Summary by CodeRabbit