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

feat(Popover): add new component #1171

Merged
merged 14 commits into from
Sep 6, 2023
Merged

feat(Popover): add new component #1171

merged 14 commits into from
Sep 6, 2023

Conversation

orest-s
Copy link
Contributor

@orest-s orest-s commented Aug 17, 2023

closes #203

@orest-s orest-s requested review from a team as code owners August 17, 2023 15:49
@github-actions
Copy link
Contributor

Preview branch generated at https://feat-add-popover.d1gko6en628vir.amplifyapp.com

Copy link
Member

@scurker scurker left a 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

docs/pages/components/Popover.mdx Outdated Show resolved Hide resolved
docs/pages/components/Popover.mdx Outdated Show resolved Hide resolved
packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
packages/styles/popover.css Outdated Show resolved Hide resolved
packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
packages/styles/popover.css Outdated Show resolved Hide resolved
packages/styles/popover.css Outdated Show resolved Hide resolved
packages/styles/popover.css Outdated Show resolved Hide resolved
packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
@orest-s orest-s requested a review from scurker August 21, 2023 16:41
Copy link
Member

@scurker scurker left a 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.

packages/styles/popover.css Outdated Show resolved Hide resolved
packages/react/__tests__/src/components/Popover/index.js Outdated Show resolved Hide resolved
packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
@orest-s orest-s requested a review from scurker August 22, 2023 11:51
Copy link
Member

@scurker scurker left a 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.

packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
@orest-s orest-s requested a review from scurker August 23, 2023 15:54
docs/pages/components/Popover.mdx Outdated Show resolved Hide resolved
docs/pages/components/Popover.mdx Outdated Show resolved Hide resolved
packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
docs/pages/components/Popover.mdx Outdated Show resolved Hide resolved
packages/styles/popover.css Outdated Show resolved Hide resolved
packages/react/src/index.ts Outdated Show resolved Hide resolved
packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
@scurker scurker dismissed their stale review August 28, 2023 14:08

completed

Copy link
Member

@scurker scurker left a comment

Choose a reason for hiding this comment

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

This one is almost good to go! Just need to fix the merge conflict and looks like the layout of the prompt popover doesn't match the design now:

Apply/close buttons are showing on the same line as the info text

scurker
scurker previously approved these changes Aug 29, 2023
@scurker
Copy link
Member

scurker commented Aug 30, 2023

@orest-s If everything is good here, feel free to squash and merge when you're ready.

@scurker scurker merged commit b09c14c into develop Sep 6, 2023
7 checks passed
@scurker scurker deleted the feat/add-popover branch September 6, 2023 18:10
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

Preview branch generated at https://feat-add-popover.d1gko6en628vir.amplifyapp.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Tooltip dialog) Popover alert component
2 participants