docs: new TopNav API #2386

anuraghazra · 2024-10-23T07:26:57Z

View Formatted Doc

changeset-bot · 2024-10-23T07:27:01Z

⚠️ No Changeset found

Latest commit: c35ab88

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

github-actions · 2024-10-23T07:27:11Z

✅ PR title follows Conventional Commits specification.

codesandbox-ci · 2024-10-23T07:30:17Z

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 cf4f0a4:

Sandbox	Source
razorpay/blade: basic	Configuration

codesandbox-ci · 2024-10-23T07:33:34Z

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 c35ab88:

Sandbox	Source
razorpay/blade: basic	Configuration

rzpcibot · 2024-10-23T07:40:01Z

Bundle Size Report

No bundle size changes detected.

Generated by 🚫 dangerJS against c35ab88

packages/blade/src/components/TopNav/_decisions/decisions.md

saurabhdaware · 2024-10-24T09:03:25Z

packages/blade/src/components/TopNav/_decisions/decisions.md

+  {({ items, moreItems }) => {
+    return (
+      <>
+        <TabNavItems>


Good API 🫡

thanks to @chaitanyadeorukhkar for the proposal. 🚀

packages/blade/src/components/TopNav/_decisions/decisions.md

saurabhdaware

Looks good

saurabhdaware · 2024-10-25T05:59:23Z

packages/blade/src/components/TopNav/_decisions/anatomy.png

nice annotations 🫡

packages/blade/src/components/TopNav/_decisions/decisions.md

vinilvasani · 2024-10-24T14:03:52Z

packages/blade/src/components/TopNav/_decisions/decisions.md

+
+```jsx
+<TabNav
+  items={[


why do we need to both pass the items here and then also specific which component to use to render this data? 🤔

Since the structure of the TabNav is well defined, with TabsNavItems and Menu, we should be able to do away with one of these 2 things right?

Defining both seems a little verbose and repetitive. Let me know what do you think

why do we need to both pass the items here and then also specific which component to use to render this data? 🤔

Why was this prop-based API chosen over the old compound component API?

Since the structure of the TabNav is well defined, with TabsNavItems and Menu, we should be able to do away with one of these 2 things right?

Tho structure is well defined, the items needs to be dynamically move around depending on screen size, TopNav's interaction requires us to control what items are shown horizontally and what items goes inside the "more" menu dynamically depending on the screen width.

And since we want to keep the flexibility of letting consumers choose what/how things gets rendered inside the More menu we need to accept the data item object instead of using compound API.

For example:
An item say "Payroll" doesn't have "description" prop when rendered in the horizontal manner but while inside the "More" menu it's whole layout changes and it also needs to render that "description", where will we get this "description" from tho, we get it form the items array.

Check the alternative APIs section for more detailed problem statement and approaches we tried out.

Screen.Recording.2024-10-29.at.11.57.54.AM.mov

looks nice and clean sir 🫡
I've dropped a few more comments regarding this, I'm not able to currently see a case where we need to provide users the flexibility of what needs to be rendered

vinilvasani · 2024-10-28T12:41:53Z

packages/blade/src/components/TopNav/_decisions/decisions.md

+            </Menu>
+          </>
+        );
+      }}


@anuraghazra how does the TopNav actions behave on mobile this is the behavior that we are expecting in the connected dashboard OneNav? Is there a way to acheive this?

https://www.figma.com/design/aMbAFFIEXn9GuFsQtLlJ5x/Connected-Dashboard%3A-Delivery-File?node-id=3658-54278&t=tDrPOotRC6L9nIfb-4

Possible.

https://github.com/razorpay/blade/blob/anu/tabnav-newapi/packages/blade/src/components/TopNav/_decisions/decisions.md#mobile-ux

Okay so it's on the consumers to define what happens on mobile?

Also what about the search on mobile? Are the consumers supposed to handle that as well?

The behaviour across viewports is well defined in the design, can we not leverage that as source of truth and handle the mobile variant accordingly from inside the component, without having the consumers define the behaviour?

Okay so it's on the consumers to define what happens on mobile?

Yes.

Also what about the search on mobile? Are the consumers supposed to handle that as well?

The search input itself can be easily added as part of the TopNav or even in the page itself since we support slots.

As to what happens on clicking the search it's product side decision, whether they want to open a modal/bottomsheet etc. This can be achieved on consumer end with existing components.

vinilvasani · 2024-10-28T12:45:06Z

packages/blade/src/components/TopNav/_decisions/decisions.md


 Mobile Navigation Bar:

-<img width="50%" src="./top-nav-mobile-example.png" alt="TopNav Mobile Example" />
+<img width="50%" src="./top-nav-mobile-example-new.png" alt="TopNav Mobile Example" />


is TopNavBrand hidden on mobile? Also does TopNavContent handle the required alignment/spacing on mobile?

Given most of the things are not even present in mobile for topnav it's recommended use Adaptive layout.

isMobile ? <TopNav>mobile items/icons/avatar</TopNav> : <TopNav> <TopNavBrand /> <TopNavContent /> <TopNavActions /> </TopNav>

We've added examples and code in storybook.

can you elaborate a little by what do you mean by use Adaptive layout, not able to understand it in the context on TopNav

Example of Adaptive Layout:

On Desktop: "show Modal"
On Mobile: "show BottomSheet"

Example of traditional responsiveness

On Desktop: "Show Modal"
On Mobile: "Shrink Modal's size"

In an adaptive layout or component you sometimes totally change what components are rendered and cater to specific UX requirements (eg: BottomSheet on mobile, modal on desktop)

packages/blade/src/components/TopNav/_decisions/decisions.md

vinilvasani · 2024-10-28T12:55:41Z

packages/blade/src/components/TopNav/_decisions/decisions.md

+```jsx
+<TabNav
+  items={[
+    { href: '/home', title: 'Home' },


how does one redirect to an external link?
Also is react-route a peer depenceny?

No peer dep, TabNavItem behave as normal links only. We have href/target etc props.

TabNavItem can be enhanced to work with react router, we have examples in storybook.

how is that possible? can you please share a link for the example? are you suggesting that we can pass from react router to TabNavItem?

We ensure we accept props that react router requires, eg as href, onClick etc.

const TabNavItemLink = React.forwardRef< HTMLAnchorElement, Omit<TabNavItemProps, 'as'> & { activeOnLinks?: string[]; } >((props, ref) => { const location = useLocation(); return ( <TabNavItem ref={ref} {...props} as={Link} isActive={isItemActive(location, { href: props.href, activeOnLinks: props.activeOnLinks })} /> ); });

vinilvasani · 2024-10-28T12:57:20Z

packages/blade/src/components/TopNav/_decisions/decisions.md

+  {({ items, overflowingItems }) => {
+    return (
+      <>
+        <TabNavItems>


what is this wrapper component for?

It holds all the horizontal items and by it's width we also determine the overflowing behaviour.

Makes sense, was wondering why we added an additional component since it wasn't present before right?
Will this be a blade component alongside TabNavItem which will be exposed to the consumer?

As i said

it's width we also determine the overflowing behaviour.

This is the container that holds your items and when it's width shrinks we automatically detect it via the ResizeObserver.

vinilvasani · 2024-10-29T07:41:05Z

packages/blade/src/components/TopNav/_decisions/decisions.md

@@ -264,6 +473,298 @@ const Dashboard = () => {
  - The navigation bar will have `role=navigation` to indicate that it is a navigation landmark.


can we have an announcement for screen readers when a TabNav item is clicked?

We will have proper announcements on focus.

Clicking generally doesn't require announcements, because on route change the router itself will announce any page state changes.

vinilvasani · 2024-10-29T07:54:17Z

packages/blade/src/components/TopNav/_decisions/decisions.md

+**Pros:**
+1. Users have easy access to the overflowingItems via props
+2. Intuitive and faimilar API, similar to Table API
+3. Users have the flexibility to render TabNavItemLinks or any other component.


why do we want to provide users with the flexibility to render anything? I think it is safe to assume that TabNav will always have a TabNavItem and a Menu right?

The Atlassian DS follows only the prop based approach which makes it less verbose overall -> https://atlassian.design/components/atlassian-navigation/examples

I think we can exercise some governanace here to follow consistency

why do we want to provide users with the flexibility to render anything? I think it is safe to assume that TabNav will always have a TabNavItem and a Menu right?

It's about what's inside the Menu. We don't know.
And we had a design discussion also that even pingal is not sure what might change in future.
The layout could change (eg: two column layout inside Menu or a product specific column layout, like 1 column for payments, 1 column for payroll), menu's header/footer etc can change.

We don't want to keep a hard dependency here because otherwise there will be a lot of blockers on product side if something changes and will introduce breaking change.

Having this flexible means product can decide what/how and where to put things.

vinilvasani · 2024-10-29T08:16:08Z

packages/blade/src/components/TopNav/_decisions/decisions.md

+      href="/payments"
+      icon={MagicCheckoutIcon}
+      // this meta data will be passed to `items` array
+      overflowMetadata={{


here we don't need this meta data right? Only thing different is the description and we can pass a description to all TabNavItemLinks and only show it once it is in more?

This is only an alternative API we explored and had discussions around, having description as a different prop also is considered in other APIs.

vinilvasani · 2024-10-29T08:50:40Z

packages/blade/src/components/TopNav/_decisions/decisions.md

+This is similar to Dropdown where on initial render we loop over all the JSX elements build a list and store it and from that list we render.
+
+**Not leaning towards this API mainly because:**
+1. JSX as Data gets very [complicated](https://github.com/razorpay/blade/blob/2e4b2cb309cdf3e87982cda9252e9ce97747c475/packages/blade/src/components/ActionList/actionListUtils.ts#L137-L159) as seen on Dropdown, and most of the time breaks composition, like it looks like the component is flexible and composable but in reality it isn’t.


can you shed some more light on why the component is not flexible and composable?

TLDR;

In dropdown the problem is we do composition via React.Children.map & check for the item's componentIds and if we don't find it we throw error, problem with this is it sort of breaks composability where now if you wrap the Dropdown items in any other wrapper component it will stop working.

vinilvasani · 2024-10-29T08:54:55Z

packages/blade/src/components/TopNav/_decisions/decisions.md

+**Not leaning towards this API mainly because:**
+1. JSX as Data gets very [complicated](https://github.com/razorpay/blade/blob/2e4b2cb309cdf3e87982cda9252e9ce97747c475/packages/blade/src/components/ActionList/actionListUtils.ts#L137-L159) as seen on Dropdown, and most of the time breaks composition, like it looks like the component is flexible and composable but in reality it isn’t.
+2. Sets a odd expectation, where users need to pass the description/icon etc props to TabNavItemLink which that component may not even use but rather they are treated as “data” attributes for the overflowingItems 
+3. We need to get the overflowingItems from somewhere, it will either be as a controlled state or we may need to introduce a render prop inside <TabNav>


didn't quite get this, if JSX is action as data can pass one more prop to mark an item as initially overflowing and then inside the component construct the data that we are passing through props right?

Didn't quite understood what you didn't understood. :p

anuraghazra added 2 commits October 23, 2024 12:15

chore: update tab nav api

3e09b07

chore: add new image

cf4f0a4

anuraghazra requested review from chaitanyadeorukhkar, kamleshchandnani, snitin315 and saurabhdaware as code owners October 23, 2024 07:26

chore: update image

de27e7c

anuraghazra added the Review - L1 First level of review label Oct 23, 2024

kamleshchandnani reviewed Oct 24, 2024

View reviewed changes

saurabhdaware reviewed Oct 24, 2024

View reviewed changes

packages/blade/src/components/TopNav/_decisions/decisions.md Outdated Show resolved Hide resolved

saurabhdaware reviewed Oct 24, 2024

View reviewed changes

packages/blade/src/components/TopNav/_decisions/decisions.md Outdated Show resolved Hide resolved

anuraghazra added 3 commits October 24, 2024 15:26

chore: updated comments

23f0271

chore: update

6e124b6

chore: update

61a5d4c

saurabhdaware reviewed Oct 25, 2024

View reviewed changes

anuraghazra added 3 commits October 25, 2024 12:25

chore: update prop

0f46955

chore: update mobile ux

289a1d9

chore: update to title

c35ab88

vinilvasani reviewed Oct 28, 2024

View reviewed changes

vinilvasani reviewed Oct 29, 2024

View reviewed changes

saurabhdaware approved these changes Oct 29, 2024

View reviewed changes

anuraghazra merged commit f4b355c into master Oct 29, 2024
15 checks passed

anuraghazra deleted the anu/tabnav-newapi branch October 29, 2024 11:00

		@@ -264,6 +473,298 @@ const Dashboard = () => {
		- The navigation bar will have `role=navigation` to indicate that it is a navigation landmark.

docs: new TopNav API #2386

docs: new TopNav API #2386

Conversation

anuraghazra commented Oct 23, 2024 • edited Loading

changeset-bot bot commented Oct 23, 2024 • edited Loading

⚠️ No Changeset found

github-actions bot commented Oct 23, 2024 • edited Loading

codesandbox-ci bot commented Oct 23, 2024

codesandbox-ci bot commented Oct 23, 2024 • edited Loading

rzpcibot commented Oct 23, 2024 • edited Loading

Bundle Size Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabhdaware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraghazra Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinilvasani Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraghazra commented Oct 23, 2024 •

edited

Loading

changeset-bot bot commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited

Loading

codesandbox-ci bot commented Oct 23, 2024 •

edited

Loading

rzpcibot commented Oct 23, 2024 •

edited

Loading

anuraghazra Oct 29, 2024 •

edited

Loading

vinilvasani Oct 29, 2024 •

edited

Loading