fix(styles): resolve bottom navbar button positioning and layout jump issues #5019
Open
FussuChalice wants to merge 10 commits intosws2apps:mainfrom
Open
fix(styles): resolve bottom navbar button positioning and layout jump issues #5019FussuChalice wants to merge 10 commits intosws2apps:mainfrom
FussuChalice wants to merge 10 commits intosws2apps:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors NavBarButton (icon-only rendering, accessibility, styling, keyboard activation), updates hook signatures (useBreakpoints exposes tablet688Up; useCurrentUser no longer returns isPersonEditor), changes NavBarOptionsType.buttons to ReactNode, adds markLastNavBarButton to useNavbar and uses it when rendering navbar buttons, and updates several page usages accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/components/nav_bar_button/index.tsx (1)
60-63:MuiTouchRippleselectors are dead CSS on a plainBox.A
Boxrenders a<div>and does not include MUI's ripple effect. These selectors will never match any child elements. Consider removing them to avoid confusion, or switch toButtonBaseif you want ripple.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/nav_bar_button/index.tsx` around lines 60 - 63, The CSS rules targeting '.MuiTouchRipple-ripple .MuiTouchRipple-child' are dead because NavBarButton renders a plain Box (no MUI ripple DOM); remove those ripple-specific selectors from the style block in the NavBarButton component, or if you intend to keep the ripple effect replace the root Box with MUI's ButtonBase (or Button) so the ripple DOM exists and update styles to target the component's ripple classes; update the code paths around the NavBarButton export and any usages of the component accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/nav_bar_button/index.tsx`:
- Line 74: The onClick prop currently sets onClick={!disabled && props.onClick},
which can pass false and is type-unsafe; change the logic to conditionally pass
undefined when disabled (e.g., use a ternary or logical check to set onClick to
undefined when disabled, otherwise props.onClick) and ensure the onClick prop
type of the component (props.onClick / NavBarButton's prop interface) allows
undefined (React.MouseEventHandler | undefined) so TypeScript strict checks
pass.
- Around line 48-77: The icon-only Box (role="button") is not
keyboard-accessible; update the Box component to include tabIndex={0} when not
disabled, an onKeyDown handler that triggers props.onClick for Enter (key ===
'Enter') and Space (key === ' ' or key === 'Spacebar') while preventing
activation when disabled, and add aria-disabled={disabled} to expose state to
assistive tech; also add cursor: 'pointer' when enabled and cursor: 'default'
when disabled in the sx styles so the visual affordance matches (refer to the
Box with role="button", props.onClick, and disabled), ensuring you still guard
calls to props.onClick so they never fire when disabled.
In `@src/features/persons/export_persons/index.tsx`:
- Around line 13-23: The NavBarButton is still clickable during processing
because disabled={isProcessing} was removed and handleExport has no re-entry
guard; restore disabled={isProcessing} on the NavBarButton (the element
rendering IconLoading/IconExport) and update handleExport to immediately return
if isProcessing is true (or otherwise guard/set a local lock) before setting
isProcessing to true so duplicate exports are prevented.
In `@src/pages/persons/person_details/index.tsx`:
- Line 41: The buttons prop is using a redundant ternary that renders the same
component in both branches; locate the JSX where buttons={laptopUp ?
<PersonButtonActions /> : <PersonButtonActions />} and replace the ternary with
a single component instance (buttons={<PersonButtonActions />}) so the code is
simpler and clearer while preserving behavior.
---
Nitpick comments:
In `@src/components/nav_bar_button/index.tsx`:
- Around line 60-63: The CSS rules targeting '.MuiTouchRipple-ripple
.MuiTouchRipple-child' are dead because NavBarButton renders a plain Box (no MUI
ripple DOM); remove those ripple-specific selectors from the style block in the
NavBarButton component, or if you intend to keep the ripple effect replace the
root Box with MUI's ButtonBase (or Button) so the ripple DOM exists and update
styles to target the component's ripple classes; update the code paths around
the NavBarButton export and any usages of the component accordingly.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pages/persons/person_details/index.tsx (1)
31-37: Consider extracting the60pxmagic number into a shared constant or CSS variable.The
60pxbottom padding is presumably tied to the bottom navbar height. If the navbar height changes, this value will silently go out of sync. A shared constant (e.g.,BOTTOM_NAVBAR_HEIGHT) or a CSS custom property would keep these coupled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/persons/person_details/index.tsx` around lines 31 - 37, Extract the hard-coded '60px' used in the Box's paddingBottom (when computing paddingBottom with tablet688Up) into a shared constant or CSS custom property (e.g., BOTTOM_NAVBAR_HEIGHT) and use that constant instead of the literal string; update the Box usage in this file to reference BOTTOM_NAVBAR_HEIGHT and add the constant to a shared UI constants module or theme (so the bottom navbar height stays in sync across components using paddingBottom and the navbar component itself).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pages/persons/person_details/index.tsx`:
- Around line 31-37: Extract the hard-coded '60px' used in the Box's
paddingBottom (when computing paddingBottom with tablet688Up) into a shared
constant or CSS custom property (e.g., BOTTOM_NAVBAR_HEIGHT) and use that
constant instead of the literal string; update the Box usage in this file to
reference BOTTOM_NAVBAR_HEIGHT and add the constant to a shared UI constants
module or theme (so the bottom navbar height stays in sync across components
using paddingBottom and the navbar component itself).
FussuChalice
changed the title
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/nav_bar_button/index.tsx (1)
48-84:⚠️ Potential issue | 🟡 MinorAvoid passing
nulltoonClickin strict TS.Line 83 assigns
nullwhen disabled, which is not assignable toMouseEventHandler | undefinedunderstrictNullChecks. Preferundefinedto keep typings strict-safe.✅ Proposed fix
- onClick={!disabled ? props.onClick : null} + onClick={!disabled ? props.onClick : undefined}#!/bin/bash # Check whether strictNullChecks is enabled in any tsconfig file. fd -t f 'tsconfig.*\.json' -E node_modules -E dist -E build | while read -r f; do echo "## $f" rg -n '"strictNullChecks"|\"strict\"' "$f" || true done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/nav_bar_button/index.tsx` around lines 48 - 84, The onClick assignment on the Box currently uses onClick={!disabled ? props.onClick : null}, which breaks strict TypeScript because null isn't assignable to MouseEventHandler | undefined; update the conditional to return undefined instead of null so the type remains MouseEventHandler | undefined (i.e., set onClick to !disabled ? props.onClick : undefined), and ensure the handler references the component prop name props.onClick and the local disabled flag in the nav_bar_button component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/nav_bar_button/index.tsx`:
- Around line 48-84: The onClick assignment on the Box currently uses
onClick={!disabled ? props.onClick : null}, which breaks strict TypeScript
because null isn't assignable to MouseEventHandler | undefined; update the
conditional to return undefined instead of null so the type remains
MouseEventHandler | undefined (i.e., set onClick to !disabled ? props.onClick :
undefined), and ensure the handler references the component prop name
props.onClick and the local disabled flag in the nav_bar_button component.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/nav_bar_button/index.tsxsrc/features/persons/export_persons/index.tsxsrc/pages/meetings/midweek/index.tsxsrc/pages/meetings/weekend/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/persons/export_persons/index.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/layouts/navbar/useNavbar.tsx (1)
158-174: Normalizemainprop to ensure single primary button per group.The
markLastNavBarButtonfunction setsmain: trueon the resolved last button but doesn't clearmainfrom otherNavBarButtoninstances. Multiple buttons in the rendered group may retain pre-existingmainattributes, creating ambiguous primary state. Recommend explicitly settingmain: falseon all non-final buttons to enforce single-primary semantics.Normalization approach
- if (lastChildIndex === null && child.type === NavBarButton) { - return cloneElement(child as ReactElement<NavBarButtonProps>, { - main: true, - }); - } + if (lastChildIndex === null && child.type === NavBarButton) { + return cloneElement(child as ReactElement<NavBarButtonProps>, { + main: true, + }); + } + + if (child.type === NavBarButton) { + return cloneElement(child as ReactElement<NavBarButtonProps>, { + main: false, + }); + } ... - const updatedNested = nested.map((nestedChild, j) => + const updatedNested = nested.map((nestedChild, j) => isValidElement(nestedChild) && nestedChild.type === NavBarButton && j === lastChildIndex ? cloneElement(nestedChild as ReactElement<NavBarButtonProps>, { main: true, }) + : isValidElement(nestedChild) && nestedChild.type === NavBarButton + ? cloneElement(nestedChild as ReactElement<NavBarButtonProps>, { + main: false, + }) : nestedChild );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/layouts/navbar/useNavbar.tsx` around lines 158 - 174, The markLastNavBarButton logic currently only sets main: true for the last NavBarButton but doesn't clear main on others; update the mapping that processes children and nested children (the blocks that check child.type === NavBarButton and the nested.map creating updatedNested) to always clone NavBarButton elements with an explicit main property: set main: true when index === lastChildIndex and set main: false for all other NavBarButton clones (use cloneElement on NavBarButton instances in both the top-level child handling and nested.map), ensuring only the final button retains main: true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/layouts/navbar/useNavbar.tsx`:
- Around line 158-174: The markLastNavBarButton logic currently only sets main:
true for the last NavBarButton but doesn't clear main on others; update the
mapping that processes children and nested children (the blocks that check
child.type === NavBarButton and the nested.map creating updatedNested) to always
clone NavBarButton elements with an explicit main property: set main: true when
index === lastChildIndex and set main: false for all other NavBarButton clones
(use cloneElement on NavBarButton instances in both the top-level child handling
and nested.map), ensuring only the final button retains main: true.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/nav_bar_button/index.types.tssrc/definition/app.tssrc/layouts/navbar/index.tsxsrc/layouts/navbar/useNavbar.tsxsrc/pages/meetings/midweek/index.tsx
💤 Files with no reviewable changes (1)
- src/pages/meetings/midweek/index.tsx
Member
|
@FussuChalice: will the deprecated props removed later or should be part of this PR too? |
Contributor
Author
The "main" parameter should remain, as it allows you to adjust it manually. Or do you want me to clean the buttons on all pages? |
Member
|
If so then no problem. It could be that JSDoc comment could be misleading. It would be best to remove that deprecated line. |
Contributor
Author
I did it. |
|
❌ The last analysis has failed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of change
Checklist: