-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat(Popover): add new component #1171
Conversation
Preview branch generated at https://feat-add-popover.d1gko6en628vir.amplifyapp.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid work here! Lots of comments but mostly just trying to sync up on some of the standards/practices that we have in cauldron. There's some additional comments from @bobbyomari
Some notes:
- There is a 4px gap at the top and bottom - is there a way to remove that so the gap and make the elements inside the popover set the spacing (the popover doesn't define the spacing between elements)
- I think the min-height should be removed or set to auto so that the height can just form to the contents inside?
- Set the close button to be secondary button and flip the positions so Apply is first
- spacing between the text and the button should be 8px
- spacing between both buttons should be 8px
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really close, just a few nits and one last thought.
With role="dialog"
we also need to ensure we're setting aria-hidden
correctly on elements outside of the popover. For Dialog, we have a utility for that (see example) that we could pull in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last couple of things! We should be ready to go once we get these few things addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…abelled as an example
5cd910e
to
490d68d
Compare
@orest-s If everything is good here, feel free to squash and merge when you're ready. |
Preview branch generated at https://feat-add-popover.d1gko6en628vir.amplifyapp.com |
closes #203