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

docs(Drawer): add API decision #2009

Merged
merged 12 commits into from
Feb 21, 2024
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
174 changes: 174 additions & 0 deletions packages/blade/src/components/Drawer/_decisions/decisions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
# Drawer

A drawer is a panel that slides in mostly from the right side of the screen over the existing content in the viewport. It helps in providing additional details or context and can also be used to promote product features or new products.

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

## Design
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

yup

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Example:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@saurabhdaware saurabhdaware Feb 8, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


## Proposed API

```jsx
<Drawer
isOpen={}
onDismiss={() => {}}
showOverlay
>
<DrawerHeader
Rozamo marked this conversation as resolved.
Show resolved Hide resolved
title=""
subtitle=""
leading={<StarIcon />}
titleSuffix={<Badge></Badge>}
trailing={
<Button icon={DownloadIcon} />
}
/>
<DrawerBody>
<Slot />
</DrawerBody>
Copy link
Member

Choose a reason for hiding this comment

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

No Footer?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope

<Drawer>
```

<details>
<summary><b>Full Usage Example</b></summary>

```jsx
const MyCuteDrawer = () => {
const [showDrawer, setShowDrawer] = React.useState(false);
return (
<Box>
<Button onClick={() => setShowDrawer(true)}>Open Drawer</Button>
<Drawer
isOpen={showDrawer}
onDismiss={() => {
setShowDrawer(false);
}}
>
<DrawerHeader
title="Announcements"
/>
<DrawerBody>
<FTXAnnouncement />
<RazorpayOnePromotions />
<CatPictures />
</DrawerBody>
<Drawer>
</Box>
)
}

```

</details>

### Props

```ts
type DrawerProps = {
/**
* Controlled state of drawer open or not
*/
isOpen: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would we need a separate callback for the back button?

Copy link
Member Author

Choose a reason for hiding this comment

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

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


/**
* Dismiss handler
*/
onDismiss: () => void;

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

*/
showOverlay?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

image

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Copy link
Collaborator

Choose a reason for hiding this comment

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

does the area outside the drawer remain interactive

We should ideally control this with a new prop like shouldDismissOnClickOutside

Rozamo marked this conversation as resolved.
Show resolved Hide resolved

/**
* Initial focus reference element
*/
initialFocusRef?: React.MutableRefObject<any>;
saurabhdaware marked this conversation as resolved.
Show resolved Hide resolved

/**
* children node.
*
* Supports DrawerHeader and DrawerBody
*/
children: React.ReactNode;

/**
* Override z-index of Drawer.
*
* @default 1002
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

*/
zIndex?: number;

/**
* Accessibility label for the drawer
*/
accessibilityLabel?: string;
};

type DrawerHeaderProps = {
/**
* Title of the Drawer
*/
title: string;

/**
* Subtitle of the Drawer
*/
subtitle?: string;

/**
* Leading element
*
* Icon or Asset
*/
leading?: ReactNode;

/**
* Title suffix element
*
* Badge
*/
titleSuffix?: ReactNode;

/**
* Title trailing element
*
* Link, Button[]
*/
trailing?: ReactNode;
};
```

_No alternate APIs were considered because Drawer is closer to Modal on overall meaning and API perspective so made sense to go with API that is closer to Modal, also all DS Drawer components I referenced have similar API_

## Drawer Stacking

- Only 2 Drawers can be stacked on top of each other
- 2nd Drawer always has overlay independent of `showOverlay` prop
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

- 1st Drawer peeks from behind with 16px gap when 2nd Drawer is opened

## 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 and Back Icon is focussable
- **Keyboard Handling:** Pressing `ESC` should close the drawer

Will work in a similar manner as [Ant Design - Drawer](https://ant.design/components/drawer)

## References

- https://atlassian.design/components/drawer/examples
- https://ant.design/components/drawer

## Open Questions

- **Design:** Should 2nd Drawer have back button or should it continue to have close button ([Context](https://github.com/razorpay/blade/pull/2009#discussion_r1487305755))
- We won't have back button anymore. We will have 1st Drawer peek from behind
- **Dev:** Should there be a prop for dismissing component on outside click. E.g. `shouldDismissOnOutsideClick` or should we handle it as part of controlled component (with the UX that confirmation modal is always shown irrespective of whether its outside click or close button click)
- We won't have new prop. This can be achieved with controlled component
- **Dev:** Should we build components like `DrawerHeaderIcon`, `DrawerHeaderAsset` (similar to `ActionListItemIcon`, `CardHeaderIcon`) or should rely on consumer to pass correct sizes and colors without building additional wrappers (similar to `DropdownHeader`, `ModalHeader`, `BottomSheetHeader`)? ([Discussion](https://razorpay.slack.com/archives/G01B3LQ9H0W/p1707822018408929))
- We won't build new components. We will follow what we follow on ModalHeader, BottomSheetHeader, etc
Loading