docs(Drawer): add API decision #2009

saurabhdaware · 2024-02-08T05:38:25Z

✨ Pretty Doc ✨

changeset-bot · 2024-02-08T05:38:29Z

⚠️ No Changeset found

Latest commit: 7831003

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-02-08T05:40:34Z

✅ PR title follows Conventional Commits specification.

codesandbox-ci · 2024-02-08T05:56:12Z

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

Sandbox	Source
razorpay/blade: basic	Configuration

chaitanyadeorukhkar · 2024-02-08T06:00:56Z

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

+   *
+   * Also decides if clicking outside on overlay closes the drawer or not
+   */
+  showOverlay?: boolean;


So overlay is optional? Won't that look weird? What is the use case?

We have usecases where Drawer is used for Announcements. In those cases, users should still be able to interact with dashboard items even with Drawer open so we cant show overlay or hide drawer on clicking outside.

when overlay is disabled, does the area outside the drawer remain interactive?

does the area outside the drawer remain interactive

We should ideally control this with a new prop like shouldDismissOnClickOutside

chaitanyadeorukhkar · 2024-02-08T06:01:33Z

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

+
+<img width="500px" src="./2024-02-07-20-04-07.png" alt="Drawer Figma Skeleton">
+
+## Design


Will Drawer be absolute positioned always? What if it needs to be used as a left/right navigation drawer?

It will be fixed positioned on top right. It is always positioned like navigation drawer

So we are setting a default position for Drawers on the top-right across all products? Like we set bottom-left for toasts?

There are use-cases where drawer is used for navigation on left side in a lot of apps. Won't that ever be a use-case for us?

There are use-cases where drawer is used for navigation on left side in a lot of apps. Won't that ever be a use-case for us?

Nope. So for desktop, our navigation bar itself is on left so there won't be anything opening from that side.

On mobile too we have drawers opening from right side only. Plus navigation component is something that we might tackle separately. This navigation component will use our Drawer internally on mobile. So at that time we can relook at this.

Anyway even if usecase comes up, its just addition of new prop so not much effort and no breaking change

chaitanyadeorukhkar · 2024-02-08T06:05:10Z

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

+  /**
+   * Show or hide overlay.
+   *
+   * Also decides if clicking outside on overlay closes the drawer or not


Wont there ever be a possibility where overlay isn't needed but click outside still needs to close the drawer?

no I did ask this to designers. If someone wants clicking outside to close drawer, they should always have overlay

Also decides if clicking outside on overlay closes the drawer or not

I feel we should have a separate prop for deciding this, it shouldn't be dependant on the presence of overlay.

you mean for a usecase where someone doesn't want to show overlay but still want to close Drawer on clicking outside? I can confirm this once with design if this should be possible. Ideally overlay should be present in most cases except few exceptional cases where outer items have to be interactive

I would also vote for the separation of the logic to dismiss the overlay items on click outside on a different prop. If Modal and BottomSheet have a prop tomorrow to disable dismiss on click outside on the overlay the same prop should be extended over here.

Agreed. Better to have a separate prop since we might be adding the same to Modal & BottomSheet too

Modal and BottomSheet would anyway have different prop than Drawer in this particular case no?

E.g. in Modal and BottomSheet, we always want to show overlay so there will never be a showOverlay prop. Whereas in Drawer, we definitely need to have showOverlay prop.

If we're having that prop and we know interactions outside have to be consistent with display of overlay, wouldn't adding new prop be extra overhead for consumers here?

Because if we add 2 props, it creates impossible states of

<> <Drawer showOverlay={true} shouldDismissOnOutsideClick={false} /> <Drawer showOverlay={false} shouldDismissOnOutsideClick={true} /> </>

These scenarios should throw error in 2 props case because its not correct usage from UX perspective

Oh anyway just discussed this with design - https://razorpay.slack.com/archives/G01B3LQ9H0W/p1707808893726369

Usecase that we have on Modal is something that we would have it here as well. I'll make changes to add 2 props.

chaitanyadeorukhkar · 2024-02-08T06:08:29Z

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

+- DrawerHeaderBadge
+- DrawerHeaderIcon
+- DrawerHeaderAsset
+- DrawerHeaderLink
+- DrawerHeaderButton


We keep creating these components for multiple components... Modal, Bottomsheet, Drawer. We might need to look into changing these into just HeaderBadge, HeaderIcon, etc. This would keep adding unnecessary repetitive components.

true. HeaderBadge is a good idea. Lets discuss this with team once in next standup?

How are these components different from normal Badge/Icon? For example, I see _CardHeaderBadge is same as Badge. Why can't we use theBadge component directly?

blade/packages/blade/src/components/Card/CardHeader.tsx

Line 48 in ba5ba9b

const _CardHeaderBadge = (props: BadgeProps): React.ReactElement => {

These subcomponents usually have defined defaults that are different from defaults of actual component. E.g.

blade/packages/blade/src/components/ActionList/ActionListItem.tsx

Line 214 in ba5ba9b

return <Badge size="medium" marginLeft="spacing.3" {...props} />;

However in some examples like CardHeaderBadge, the defaults of wrapper are same as defaults of actual component. There it is just for consistency in API otherwise some components will be like Badge while some be like CardHeaderText etc

chaitanyadeorukhkar · 2024-02-08T06:09:38Z

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

+## Accessibility
+
+- **Aria Attributes:** Drawer will have `aria-modal="true"` and `role="dialog"` and will be treated as modal for voiceover users.
+- **Focus Handling:** Ensure Close Icon is focussable


Like Modal & Bottomsheet, should we also expose initialFocusRef?

Yes I think we should. Drawer will also have similar usecases like Modal and BottomSheet so requirement will anyway come

Add keyboard handling details, e.g, Esc should close the drawer.

chaitanyadeorukhkar · 2024-02-08T06:11:33Z

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

+
+## Design
+
+- [Drawer - Figma Design](https://www.figma.com/file/jubmQL9Z8V7881ayUD95ps/Blade-DSL?node-id=78667%3A66663&mode=dev)


Does the drawer have a fixed width? Will the width change based on the content? If it is fixed width, why do we not have size options?

It currently has a fixed max-width of 420px and inside that it takes width based on the content.

Let me check with design once if we want to go with max-width or defined size because in Modal I see its defined size

Just checked with RK. It is fixed width of 420px on desktop always. We will only support one size right now

fixed width of 420px
Max width would be 420px right?

nope. Drawer is always 420px irrespective of the content.

chaitanyadeorukhkar · 2024-02-08T06:12:21Z

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

+  /**
+   * Controlled state of drawer open or not
+   */
+  isOpen: boolean;


What happens if multiple drawers are opened together? I'm guessing same behaviour as Modal?

Yes similar but only 2 drawers can be kept open at once from design-side. And the 2nd drawer has a back button instead of close button.

Let me add the stacking behaviour to this API doc I didn't mention it in this

Would we need a separate callback for the back button?

back button is just dismiss of 2nd Drawer. Consumer knows which is their second drawer so they can use that onDismiss itself.

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

snitin315 · 2024-02-09T06:45:22Z

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

+
+<img width="500px" src="./2024-02-07-20-04-07.png" alt="Drawer Figma Skeleton">
+
+## Design


So we are setting a default position for Drawers on the top-right across all products? Like we set bottom-left for toasts?

snitin315 · 2024-02-09T06:50:00Z

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

+  /**
+   * Show or hide overlay.
+   *
+   * Also decides if clicking outside on overlay closes the drawer or not


Also decides if clicking outside on overlay closes the drawer or not

I feel we should have a separate prop for deciding this, it shouldn't be dependant on the presence of overlay.

snitin315 · 2024-02-09T06:54:48Z

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

+- DrawerHeaderBadge
+- DrawerHeaderIcon
+- DrawerHeaderAsset
+- DrawerHeaderLink
+- DrawerHeaderButton


How are these components different from normal Badge/Icon? For example, I see _CardHeaderBadge is same as Badge. Why can't we use theBadge component directly?

blade/packages/blade/src/components/Card/CardHeader.tsx

Line 48 in ba5ba9b

const _CardHeaderBadge = (props: BadgeProps): React.ReactElement => {

snitin315 · 2024-02-09T06:56:02Z

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

+## Accessibility
+
+- **Aria Attributes:** Drawer will have `aria-modal="true"` and `role="dialog"` and will be treated as modal for voiceover users.
+- **Focus Handling:** Ensure Close Icon is focussable


Add keyboard handling details, e.g, Esc should close the drawer.

Co-authored-by: Nitin Kumar <snitin315@gmail.com>

…zorpay/blade into drawer/api-decision

nashcheez · 2024-02-12T16:13:08Z

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

+  /**
+   * Show or hide overlay.
+   *
+   * Also decides if clicking outside on overlay closes the drawer or not


I would also vote for the separation of the logic to dismiss the overlay items on click outside on a different prop. If Modal and BottomSheet have a prop tomorrow to disable dismiss on click outside on the overlay the same prop should be extended over here.

nashcheez · 2024-02-12T16:15:32Z

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

+  /**
+   * Override z-index of Drawer.
+   *
+   * @default 1002


@saurav12 Can design take up a decision on the alignment of all our default z-indices for our overlay components? Imo it should be Modal < Drawer < BottomSheet < Popover < Tooltip. We have the zIndex property to override for each component but by default the system should make a recommendation.

Sure, makes sense. We will document these in the Figma itself.

snitin315 · 2024-02-13T06:10:56Z

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

+  />
+  <DrawerBody>
+    <Slot />
+  </DrawerBody>


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

snitin315 · 2024-02-13T06:19:54Z

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

+## Drawer Stacking
+
+- Only 2 Drawers can be stacked on top of each other
+- 2nd Drawer always has overlay independent of `showOverlay` prop


So, the showOverlay prop is redundant for the 2nd drawer? Why so? If I don't want to show overlay in 1st drawer because important information is present on the left then that should hold for the 2nd drawer as well.

So I feel, if we are giving control of showing overlay it should be either on both drawers or none. WDYT?

So, the showOverlay prop is redundant for the 2nd drawer? Why so? If I don't want to show overlay in 1st drawer because important information is present on the left then that should hold for the 2nd drawer as well.

For example in X, 1st Drawer is used for showing information based on the interaction on left side. But 2nd Drawer does not have that usecase.

Plus overlay on 2nd Drawer is also there to denote the stacking. If there's 2 Drawers open without overlay, there is no direct way to tell user that there is another drawer behind this drawer.

Also in most cases, Drawer should always have overlay. There are just few exceptions for 1st Drawer which is why we need to provide overlay prop

snitin315 · 2024-02-13T06:20:04Z

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

+- 2nd Drawer always has overlay independent of `showOverlay` prop
+- 2nd Drawer always has back button instead of close button. Clicking on back button closes the 2nd drawer.
+
+<img width="500px" src="./2024-02-12-11-01-32.png" />


Can you add a code example for stacked drawers?

Good point. Adding.

chaitanyadeorukhkar · 2024-02-13T07:00:07Z

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

+## Drawer Stacking
+
+- Only 2 Drawers can be stacked on top of each other
+- 2nd Drawer always has overlay independent of `showOverlay` prop


Won't this be counterintuitive? How will we communicate this to our devs? I can imagine this coming in as a bug in the future

Won't this be counterintuitive?

True but don't see any other way to implement this. We do want to show overlay on 2nd drawer.

#2009 (comment)

How will we communicate this to our devs?

We can throw error. Also its enforced from design side so its not possible to implement something like this on design as the UI itself is slightly different for 2nd drawer.

chaitanyadeorukhkar · 2024-02-13T07:01:24Z

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

+
+- Only 2 Drawers can be stacked on top of each other
+- 2nd Drawer always has overlay independent of `showOverlay` prop
+- 2nd Drawer always has back button instead of close button. Clicking on back button closes the 2nd drawer.


Why though? a back button only makes sense when the navigation is within the drawer and not another drawer on top of it.

If we are doing this, then we should also have a back button on 2nd Modal? Same logic would be applicable there. I think we shouldn't have a back button in either cases.

In case of Modal, the size of the modal would also let people know about modal behind it no? If we put close button on 2nd Drawer, because all Drawers are same size and similarly placed, it feels like clicking close will close everything. But instead we only go back to the first Drawer in the stack.

Basically we would need something to let consumer know that this Drawer is stacked. Back button and overlay was our way to communicate that. But we can discuss this once with RK maybe?

Started discussion here - https://razorpay.slack.com/archives/G01B3LQ9H0W/p1707813910309619?thread_ts=1707803235.144969&cid=G01B3LQ9H0W

Adding it to open questions

…cision

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

snitin315

* feat(Drawer): add API decisions * feat: add full skeleton * fix: image width * fix: typo * feat: add stacking section to api decision * Update packages/blade/src/components/Drawer/_decisions/decisions.md Co-authored-by: Nitin Kumar <snitin315@gmail.com> * fix: remove DrawerHeaderLink and DrawerHeaderButton * feat: add open questions * feat: add discussion slack thread * feat: update API decision with new API --------- Co-authored-by: Nitin Kumar <snitin315@gmail.com>

saurabhdaware added 2 commits February 7, 2024 21:15

feat(Drawer): add API decisions

48f6a9b

feat: add full skeleton

b5fa002

fix: image width

9555c14

saurabhdaware marked this pull request as ready for review February 8, 2024 05:42

saurabhdaware requested review from chaitanyadeorukhkar and kamleshchandnani as code owners February 8, 2024 05:42

fix: typo

04d4444

chaitanyadeorukhkar reviewed Feb 8, 2024

View reviewed changes

snitin315 reviewed Feb 9, 2024

View reviewed changes

saurabhdaware and others added 2 commits February 12, 2024 11:02

feat: add stacking section to api decision

b70b70d

Update packages/blade/src/components/Drawer/_decisions/decisions.md

593fe1b

Co-authored-by: Nitin Kumar <snitin315@gmail.com>

saurabhdaware requested review from chaitanyadeorukhkar and snitin315 February 12, 2024 05:35

saurabhdaware added 2 commits February 12, 2024 17:01

fix: remove DrawerHeaderLink and DrawerHeaderButton

6be7125

Merge branch 'drawer/api-dect push origin dra ision' of github.com:ra…

73bfc17

…zorpay/blade into drawer/api-decision

nashcheez reviewed Feb 12, 2024

View reviewed changes

snitin315 reviewed Feb 13, 2024

View reviewed changes

chaitanyadeorukhkar reviewed Feb 13, 2024

View reviewed changes

saurabhdaware added 3 commits February 14, 2024 10:39

feat: add open questions

6776ff4

feat: add discussion slack thread

96975bf

feat: update API decision with new API

907edf5

saurabhdaware added the Review - L1 First level of review label Feb 16, 2024

Merge branch 'master' of github.com:razorpay/blade into drawer/api-de…

7831003

…cision

saurabhdaware requested review from chaitanyadeorukhkar and snitin315 February 20, 2024 05:41

Rozamo reviewed Feb 20, 2024

View reviewed changes

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

Rozamo reviewed Feb 20, 2024

View reviewed changes

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

chaitanyadeorukhkar approved these changes Feb 21, 2024

View reviewed changes

snitin315 approved these changes Feb 21, 2024

View reviewed changes

saurabhdaware merged commit 58a6d9d into master Feb 21, 2024
14 checks passed

saurabhdaware deleted the drawer/api-decision branch February 21, 2024 05:43


		<img width="500px" src="./2024-02-07-20-04-07.png" alt="Drawer Figma Skeleton">

		## Design


		## Design

		- [Drawer - Figma Design](https://www.figma.com/file/jubmQL9Z8V7881ayUD95ps/Blade-DSL?node-id=78667%3A66663&mode=dev)

docs(Drawer): add API decision #2009

docs(Drawer): add API decision #2009

Conversation

saurabhdaware commented Feb 8, 2024 • edited Loading

changeset-bot bot commented Feb 8, 2024 • edited Loading

⚠️ No Changeset found

github-actions bot commented Feb 8, 2024 • edited Loading

codesandbox-ci bot commented Feb 8, 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

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

saurabhdaware Feb 8, 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

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

snitin315 left a comment

Choose a reason for hiding this comment

saurabhdaware commented Feb 8, 2024 •

edited

Loading

changeset-bot bot commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

codesandbox-ci bot commented Feb 8, 2024 •

edited

Loading

saurabhdaware Feb 8, 2024 •

edited

Loading