docs(SideNav): API Decision

[saurabhdaware]

✨ Formatted Document ✨

Changelog of API

First Class Tooltip support as suggested by @anuraghazra

- <Tooltip content="Something">
-   <SideNavLink />
- </Tooltip>
+ <SideNavLink tooltip={{content: 'Something'}}>

as prop on SideNavLink as suggested by @kamleshchandnani

- <SideNav routerLink={NavLink}>
-  <SideNavLink />
- </SideNav>
+ <SideNav>
+  <SideNavLink as={NavLink} />
+ </SideNav>

Active state won't be handled internally

Consumers have to explicitly mark item as Active based on their router state. Checkout formatted doc for more info

<SideNavLink
   as={NavLink}
+  isActive={true}
/>

SideNavItem component addition

Instead of asking consumers to build custom item with Box, we give paddings and default styles pre-configured in SideNavItem. We can use it for Test Mode usecase now

<SideNavItem
  leading={}
  trailing={}
  title=""
/>

SideNavBody component addition

We needed to add SideNavBody to add scroll in the items

<SideNav>
-   <SideNavLink />
-   <SideNavLink />
+  <SideNavBody>
+    <SideNavLink />
+    <SideNavLink />
+  </SideNavBody>
</SideNav>
  

[github-actions]

✅ PR title follows Conventional Commits specification.

[anuraghazra]

We can put a tooltip here as well, we should update the screenshot with an example.

[saurabhdaware]

I think both trailing and item itself can have tooltip so we can keep the one that we have below. I'll add tooltip example on trailing

[kamleshchandnani]

so all the trailing will only be visible on hover by default?

[saurabhdaware]

yes. And currently only button is accepted in trailing slot

[chaitanyadeorukhkar]

We should mention somewhere in documentation and here in prop table that trailing is only visible on hover

[anuraghazra]

This will cause a lot of issues while iterating over the nested items, because now you have to map over the Tooltip and allow it as part of the composite element.

[anuraghazra]

We can instead have this as a first class feature.

`<SideNavLink tooltip={{ content: "Tooltip content" }} />

[saurabhdaware]

This will cause a lot of issues while iterating over the nested items, because now you have to map over the Tooltip and allow it as part of the composite element.

Trying out approach that doesn't require iterating over items

[saurabhdaware]

`<SideNavLink tooltip={{ content: "Tooltip content" }} />

This makes sense. Lets go with this

[kamleshchandnani]

how can we make a particular nav item selected by default? also, it's not just default if someone lands on a URL say one merchant sharing it with another/our support team when we land on that URL the nav item shall be selected by default so shall we also have controlled API?

[saurabhdaware]

As discussed in office, even if we support adding isSelected or isActive prop, it will only select item and not change the route itself. With the current approach, they can move to their selected route and that will select item

[kamleshchandnani]

is it also possible to have an info icon and show the tooltip on that? can we add an example of that?

[kamleshchandnani]

also shall we not have tooltip on the entire nav link and have on info icon? because a11y?

[saurabhdaware]

I think it will be there on overall item (similar to our Tooltip with Button example). Because tooltip represents what happens on click of item.

Anurag suggested having tooltip inside SideNavLink component as first-class feature so will explore that

[kamleshchandnani]

what if we just have NavLink and provide onClick and href as 2 props and if devs want nested levels they can use onClick and if its href that's a navigation? its the same terminology as link as button and link as nav

[saurabhdaware]

Currently clicking item also changes the route so need href there

[kamleshchandnani]

how are we deciding when to open the sub nav(L2) and when to expand it(L3)? using context?

[saurabhdaware]

yup using context

[vinilvasani]

can you elaborate more on this?

[saurabhdaware]

Basically I pass level in context, then in next context I pass level + 1 so it gives the nesting of context providers from that I can tell if its L2 or L3

[kamleshchandnani]

we need to give a way for product teams to track analytics for every sub-component of Nav being clicked.

[saurabhdaware]

onClick on SideNavLink should be able to cover most usecases 🤔

[kamleshchandnani]

when a user clicks the +13 more we need to give them a trigger to track analytics

[saurabhdaware]

yup right. Will add this

[kamleshchandnani]

do we really need to have sidelevel here? this kind of is leading to confusion because I can't have title on level 3. Also, the SideNavLink /accounts/settings you won't navigate right when you're on L2 and want to access L3 items because in this case the Edit its just acting like a collapsible item?

[saurabhdaware]

do we really need to have sidelevel here? this kind of is leading to confusion because I can't have title on level 3

I missed removing title prop from SideNavLevel in this example. Had removed it from props table. So yup we won't have title prop in both L2 and L3.

I kept SideNavLevel here because it then follows the same structure in L2 and L3 so people can recursively loop through the object and render items without any additional logic

Also, the SideNavLink /accounts/settings you won't navigate right when you're on L2 and want to access L3 items because in this case the Edit its just acting like a collapsible item?

This is true. Not sure what is alternative. Currently I have kept href optional so if you don't have href value here, it opens collapsible without navigation. But if you have href, then it navigates and opens collapsible at the same time (kind of like https://react.dev/reference/react-dom/client)

[kamleshchandnani]

so all the trailing will only be visible on hover by default?

[chaitanyadeorukhkar]

I hope we will handle the edge case where people try to add L4 sidenav level and throw some meaningful error

[saurabhdaware]

yup added in implementation

[vinilvasani]

The API decision doc is nicely put together and was fun to read through 🚀
Have dropped a few comments

[vinilvasani]

how do compound APIs facilitate in providing higher flexibility?

[saurabhdaware]

Lets say you want to add leading, trailing, descriptions, etc to items. With config-driven API we'll have to keep on adding that to the config schema. So only things that we allow in our config schema are valid rest are invalid.

With compound API, on lower level you can put any JSX and that will be rendered. So if any new usecase comes, in a lot of scenarios we might be able to write custom JSX that allows adding that usecase without opting out of entire SideNav.

<SideNav>
  <Box>Random other text</Box>
</SideNav>

[vinilvasani]

agreed it's intuitional to read through the config and render the nested structure

[vinilvasani]

can we have a nav item change/select handler on the top-level component that can listen to selections by the user and can then be used for something like triggering relevant analytics events?

[saurabhdaware]

We have onClick on items so that can be used for analytics. E.g. If onClick on L2 trigger item is called then its L2 open event for analytics

[vinilvasani]

How will trailing and titleSuffix sit together?

[saurabhdaware]

Added the screenshots in table below. titleSuffix will come next to title and trailing will be on the right. However depending on usecase and how it looks, size of title, etc. Designers can take call whether they really want to show both or if just one is fine

[vinilvasani]

since this slot has a designated function of displaying badges and counters, instead of titleSuffix can we call it something on the lines of supplementSlot

[saurabhdaware]

titleSuffix is the standard name we follow in components like AccordionHeader, Card, DropdownHeader, etc to make API more predictable across blade so following the same thing here

[vinilvasani]

Is there a use case where we need to mark the L2 trigger as active? Won't one of the nested nav items inside of L2 be active?

[saurabhdaware]

When one of the item inside is marked active, its parent is also marked active in L1

[vinilvasani]

if maxVisibleItems is not provided will it be in expanded state by default?

[saurabhdaware]

yup in that case all items are always visible and that "Show More" link itself won't exist

[vinilvasani]

when the SideNav mounts will the initially active item be in focus by default?

[saurabhdaware]

no the focus will be controlled on page level so assuming the top-nav would have first focus and then sidenav

[vinilvasani]

what is the SkipNav utility can you share more context about it?

[saurabhdaware]

We have this utility in blade - https://blade.razorpay.com/?path=/docs/components-accessibility-skipnav--docs

So if you start tabbing, you see a Skip to content link which you can click to skip navigation and go to content directly

[vinilvasani]

NavLink internally maintains the isActive based on route defined right? Can we not make use of that?

it doesn't give us access to handling state internally for our L1 -> L2 navigations

can you please elaborate on this a little?

[saurabhdaware]

NavLink internally maintains the isActive based on route defined right? Can we not make use of that?

It doesn't give any isActive in JS / React state. It adds aria-current="page" to link which can be used for CSS styling but because we have multiple menus where lets say you want to open L2 from L1, you need a way to know which item is Active in the react state (Added a bit more context in that React Router's proposal)

NavLink internally maintains the isActive based on route defined right

Basically the issue is its "only" internally maintaining that active state inside React Router's NavLink. Its not giving us a way to access that active state in our component's state

[anuraghazra]

Why does a submenu has an href?

Shouldn't the href of the first children of this submenu be auto selected?

[saurabhdaware]

I can't tell which is the first element without doing React.Children.map which we can't do because people will create wrapper NavLink which handles react router state

[anuraghazra]

Oh so what happens if the Submenu href and the first items href doesn't match?

[saurabhdaware]

It still works. Just L2 menu won't have any selection and it will take you to submenu's href