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 popover component and initial styles #7684

Merged

Conversation

joshblack
Copy link
Contributor

@joshblack joshblack commented Jan 28, 2021

Closes #7713

This PR implements the initial spec for the Popover component, a utility for rendering UI on a layer that sits above the current UI. Currently, it handles the follow situations from the spec in the issue:

  • Variants
    • With caret
    • Without caret
    • Light
    • High contrast
  • Alignment (to the trigger element)
    • Bottom (center, left, right)
    • Top (center, left, right)
    • Right (center, top, bottom)
    • Left (center, top, bottom)
  • Dimensions
    • Width (with max)
    • Padding This will be implement for layouts that use it since the component can both render text and things like menus

Changelog

New

  • _popover.scss in packages/components
  • src/components/Popover in packages/react

Changed

  • Update our useId helper to reference canUseDOM from a shared environment.js file

Removed

@netlify
Copy link

netlify bot commented Jan 28, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit c4aee59

https://deploy-preview-7684--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Jan 28, 2021

Deploy preview for carbon-elements ready!

Built with commit c4aee59

https://deploy-preview-7684--carbon-elements.netlify.app

@joshblack joshblack marked this pull request as ready for review February 18, 2021 18:43
@joshblack joshblack requested a review from a team as a code owner February 18, 2021 18:43
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Lookin good! 🎉 I have a couple different comments around alignment and spacing:


  • When the caret is positioned top/bottom and left/right. The caret needs to be 16px from that specified edge.

16px caret edge


Examples story:

  • There is a tooltip that comes up as well as a popover. For these examples shouldn’t we only have the popover appear here?

Screen Shot 2021-02-24 at 11 28 39 AM

  • For the 48px icon buttons, for demoing purposes on this story we need to move the popover over so the caret center aligns with the button, instead of aligning to the container. I can show other different directions with specs if that would be helpful.

for demo show as intended


Playground story:

  • Can we bring the popover more into view on the page, so when people try to play with it they can see the popover better?

Screen Shot 2021-02-24 at 11 29 56 AM

@joshblack
Copy link
Contributor Author

@laurenmrice updated! I fixed the caret positioning so it should be good, I also excluded the dev-only stories like I mentioned in Slack so it should just be the playground now 👀

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Looks good to me ! 🙌🏻

@joshblack
Copy link
Contributor Author

Let me know if you have any questions @tay1orjones @emyarod @tw15egan !

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

are some of the stories missing from the storybook preview? Default and Examples aren't appearing

packages/react/src/components/Popover/Popover-story.js Outdated Show resolved Hide resolved
@include box-shadow;

position: relative;
width: max-content;
Copy link
Member

Choose a reason for hiding this comment

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

we've been avoiding this but have we already dropped support for IE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emyarod the Popover work is targetting post-IE11 (v11) stuff 👀

);
}

return (
Copy link
Member

Choose a reason for hiding this comment

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

Can all this extra stuff in the story file be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured we could keep it for now until we figure out a better alignment story that we can use to verify (or only enable ti for development but not for production kind of thing)

@joshblack
Copy link
Contributor Author

@emyarod correct, some of them are hidden since they are more-so for development working on it versus intended to be used by the end-user. I just don't think we have a good example of a story to use for alignment yet so it's just hidden for now while we figure that out 👀

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

@kodiakhq kodiakhq bot merged commit 3c819d2 into carbon-design-system:master Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[popover] basic component spec
5 participants