feat: add Drawer and Dialog components

jaredcwhite commented 1 year ago

Issue Overview

This PR addresses #46 & #25

Based on work by: @emilyjablonski in #57

Description

This PR adds two components, Drawer and Dialog. Both are lightly opinionated takes on top of a common component, Overlay. Drawer and Dialog control their own styling and can be overridden by consumers individually, but a lot of the time it's just passing Overlay defaults along.

How Can This Be Tested/Reviewed?

Storybook for Dialog: https://deploy-preview-59--storybook-ui-seeds.netlify.app/?path=/story/overlays-dialog--default

Storybook for Drawer: https://deploy-preview-59--storybook-ui-seeds.netlify.app/?path=/story/overlays-drawer--default

Checklist:

[ ] I have added QA notes to the issue with applicable URLs
[x] I have performed a self-review of my own code
[x] I have reviewed the changes in a desktop view
[x] I have reviewed the changes in a mobile view
[x] I have made corresponding changes to the documentation
[x] My changes generate no new warnings
[x] I have added tests that prove my fix is effective or that my feature works
[x] I have exported any new components
[ ] My commit message(s) and PR title are polished in the conventional commit format, and any breaking changes are indicated in the message and are well-described

netlify[bot] commented 1 year ago

Deploy Preview for storybook-ui-seeds ready!

Name	Link
Latest commit	5f9434c7b9bf572d94ac60a02f09d9704a43ff67
Latest deploy log	https://app.netlify.com/sites/storybook-ui-seeds/deploys/65380c58b26fa10008bcda75
Deploy Preview	https://deploy-preview-59--storybook-ui-seeds.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

jaredcwhite commented 1 year ago

This is in a decent place to review, but I want to run a couple questions by Jesse once he's back.

emilyjablonski commented 1 year ago

It seems like it's possible the Dialog needs a little bit more padding underneath the header in a small mobile view

I also think our current implementation of Drawer is that the footer is not sticky when it's overflowing - I prefer this implementation but wanna make it was intentional

slowbot commented 1 year ago

@jaredcwhite

Dialog

I think we want a margin-y between the dialog and the browser window until we get to the smallest screen size
At the smallest screen size we want the border-radius at 0

Drawer

Have we not added transitions yet? Do we want to spec the same animation curve for both?
Regarding @emilyjablonski comment, I also like the behavior I see here for the footer.
This may be less relevant, but I think on right-aligned actions (modal) we want primary action on the right, and on left-aligned actions (drawer) we want primary action on the left

jaredcwhite commented 1 year ago

Thanks for the feedback @slowbot. Dialog (1) and (2) sounds good.

Regarding transitions, we don't have any transitions enabled for any Seeds components so far. I'd love to tackle this in a single pass for any components we have to date which need transitions (as it's a bit tricky in React to set that up so we'll want to implement an approach once and reuse across the system.) So yes let's spec Dialog/Drawer animations and then I can file a separate issue to continue to work elsewhere (maybe Toast?).

Drawer (5): sounds good, I'll change the button order.

jaredcwhite commented 11 months ago

@emilyjablonski: It seems like it's possible the Dialog needs a little bit more padding underneath the header in a small mobile view

Ah, yeah I'm seeing that even when it's a large screen but the content is overflowing. I'll try to pull padding from the top of the content and apply to bottom of the heading instead.

jaredcwhite commented 11 months ago

All right, I think the feedback from before has been addressed now. (Also, we resolved in a design huddle to punt on transitions/animations until we look at that cross-component in a separate pass.)

jaredcwhite commented 11 months ago

@ludtkemorgan Thanks for catching the media query issue, should be fixed now. I also added a description to the comment block for the usePortal hook, let me know if you still have questions.

ColinBuyck commented 11 months ago

For the dialog, I'm noticing that the component width extends to 510 when the Figma has its max-width at 480. And for the drawer, I'm seeing it extend to 1278 as opposed to the 1024 on the the Figma.

ColinBuyck commented 11 months ago

Also seems like the drawer heading needs 8px of padding on the top to match the Figma requirements. of 24px

jaredcwhite commented 11 months ago

OK, I'll double-check all the dimensions. Generally I'm not measuring anything by px but by rem based on the size tokens we've set up. (Pixels may change based on platform, zoom level, etc.)

jaredcwhite commented 11 months ago

Hmm, now that I'm reviewing I don't see any "set" sizes for the drawer, but rather it should keep scaling full-width until it reaches roughly 1024pixels wide and then stay there. I'm not sure where all our breakpoints come from. @emilyjablonski Do you recall where the different drawer width breakpoints came from? I don't see any of those in Figma.

slowbot commented 10 months ago

@jaredcwhite Including links to breakpoint docs

Dialog does have a max width of 480px set in Figma
Drawer does have an implied max with of 1024px applied in Figma

jaredcwhite commented 10 months ago

@slowbot OK, if so that sounds good to you @emilyjablonski I'll just make sure we have a simplified max-width applied on both of those and not rely on any breakpoint-specific widths.

jaredcwhite commented 10 months ago

@emilyjablonski Cool…I think that's working as designed because the primary action should be the first one and that's on the right, but we can validate that in QA.

github-actions[bot] commented 10 months ago

:tada: This PR is included in version 1.12.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

slowbot commented 9 months ago

@jaredcwhite - Just one observation

I'm still seeing some left and right margins on small mobile Dialog. Was this intentional? I thought the designed call of edge to edge on mobile.

jaredcwhite commented 9 months ago

@slowbot Hmm, I can't repro. Looking at this: https://storybook-ui-seeds.netlify.app/?path=/story/overlays-dialog--default

and it's displaying like this (on iOS 17) where it's hugging the edges:

slowbot commented 9 months ago

@jaredcwhite Retested on my end and it looks good. Was a testing issue by me.

bloom-housing / ui-seeds