Skip to content
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

feat(SideNav): add description and onLevelChange callback #2488

Merged
merged 8 commits into from
Jan 24, 2025

Conversation

saurabhdaware
Copy link
Member

@saurabhdaware saurabhdaware commented Jan 22, 2025

Description

  • Add description for L2 items
  • Add onVisibleLevelChange callback

Changes

Additional Information

Component Checklist

  • Update Component Status Page
  • Perform Manual Testing in Other Browsers
  • Add KitchenSink Story
  • Add Interaction Tests (if applicable)
  • Add changeset

Copy link

changeset-bot bot commented Jan 22, 2025

🦋 Changeset detected

Latest commit: be18e87

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@razorpay/blade Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Jan 22, 2025

✅ PR title follows Conventional Commits specification.

Copy link

codesandbox-ci bot commented Jan 22, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit be18e87:

Sandbox Source
razorpay/blade: basic Configuration

@saurabhdaware saurabhdaware added the Review - L1 First level of review label Jan 22, 2025
@rzpcibot
Copy link
Collaborator

rzpcibot commented Jan 22, 2025

Bundle Size Report

Updated Components
Status Component Base Size (kb) Current Size (kb) Diff
SideNav, SideNavLink, SideNavLevel, SideNavSection, SideNavItem, SideNavFooter, SideNavBody, SIDE_NAV_EXPANDED_L1_WIDTH_BASE, SIDE_NAV_EXPANDED_L1_WIDTH_XL 33.382 33.609 +0.227 KB
SearchInput 34.737 34.738 +0.001 KB

Generated by 🚫 dangerJS against be18e87

@tewarig
Copy link
Contributor

tewarig commented Jan 22, 2025

Here, onLevelChange is being called twice when I am on the same level.

When I am on level 2, I hover over level 1 items, and the level gets changed, triggering onLevelChange. Then I click on a level 1 item. Although the level has not changed, onLevelChange is still being called.

SideNav.mov

*
* **Note**: Only applicable for L2 items
*/
description?: string;
Copy link
Contributor

@tewarig tewarig Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have a check for this ? we can throw warning in console if description is provided to L1 items ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. We can throw dev-only error. I was avoiding production error because if backend API mistakenly sends description then we don't want the sidenav to error out completely.

@saurabhdaware
Copy link
Member Author

When I am on level 2, I hover over level 1 items, and the level gets changed, triggering onLevelChange. Then I click on a level 1 item. Although the level has not changed, onLevelChange is still being called.

So I was kind of thinking onLevelChange as callback that also gets triggered when the active level changes. So in future we can have argument of activeLevel alongside visibleLevel. For the scenario in video, the activeLevel changes when you click the item in L1.

But yes without the activeLevel argument it looks confusing. What do you think should we add activeLevel argument right now or should we only cover visibility change as activeLevel change can also be derived from which item is selected.

@saurabhdaware
Copy link
Member Author

@tewarig Renamed the prop onVisibleLevelChange and calling it only once now when L1 is hovered and clicked

"@razorpay/blade": minor
---

feat(SideNav): add `description` on SideNavLink and `onLevelChange` callback on `SideNav`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onVisualLevelChange

anuraghazra
anuraghazra previously approved these changes Jan 23, 2025
@saurabhdaware saurabhdaware merged commit 194fffc into master Jan 24, 2025
13 of 15 checks passed
@saurabhdaware saurabhdaware deleted the feat/sidenav-enhancements branch January 24, 2025 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review - L1 First level of review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants