-
Notifications
You must be signed in to change notification settings - Fork 318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added active class to alpha mail sidebar #202
base: main
Are you sure you want to change the base?
Conversation
@Vishvsalvi is attempting to deploy a commit to the Inbox Zero Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Sidebar
participant Links
participant NavLink
User->>Sidebar: Navigates to a page
Sidebar->>Links: Fetch current path and search parameters
Links->>NavLink: Generate links with typeParams
NavLink->>Links: Determine active state based on path and typeParams
Links-->>User: Display updated navigation links
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/web/components/SideNav.tsx (4 hunks)
Additional comments not posted (4)
apps/web/components/SideNav.tsx (4)
4-4
: LGTM! The new import statement is appropriate.The
useSearchParams
hook is correctly imported fromnext/navigation
.
300-302
: LGTM! The new logic for capturing search parameters is correct.The
useSearchParams
hook is used to retrieve the "type" parameter from the URL.
338-346
: LGTM! The new logic for constructing thepath
prop is correct.The
path
prop now includes thetypeParams
, ensuring that the links reflect the current type parameter.
401-401
: Verify the new logic for determining the active state.The
isActive
variable now checks for both exact matches and partial matches. Ensure that this logic correctly identifies the active state.Verification successful
Verified the new logic for determining the active state.
The
isActive
variable now correctly checks for both exact matches and partial matches, ensuring the correct identification of the active state.
isActive
is used to determine the class of the link element, toggling between "bg-gray-800" for active links and "hover:bg-gray-800" for inactive links.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic for determining the active state of navigation links. # Test: Search for the usage of the `isActive` variable. Expect: Correct identification of active state. rg --type javascript -A 5 $'const isActive'Length of output: 79
Script:
#!/bin/bash # Description: Verify the logic for determining the active state of navigation links. # Test: Search for the usage of the `isActive` variable. Expect: Correct identification of active state. rg --type ts -A 10 $'const isActive'Length of output: 773
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question
@@ -392,14 +398,15 @@ function Links(props: { path: string; links: NavItem[] }) { | |||
|
|||
function NavLink(props: { path: string; link: NavItem }) { | |||
const { link } = props; | |||
const isActive = props.path === link.href || props.path.includes(link.href); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason for includes
here?
But also includes
would match ===
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The includes
matches the query string, ===
doesn't match the query string it is for the paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If props.path.includes(link.href)
is true then it must be that props.path === link.href
is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh I haven't checked that yet! If that's the case I'll make the changes
The active class remains on the sidebar
Summary by CodeRabbit
New Features
Bug Fixes