-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add 'Popover' component #383
Conversation
Also fixed the naming of the `Fixes: #` with the help of @shawnbot.
Improve Pull Request template
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.
Left a few comments but as you said you are making some more changes I'll stop for now. Let me know when you're ready for a review later!
top: -15px; | ||
margin-left: -9px; | ||
border: 8px solid transparent; | ||
border-bottom-color: rgba(0, 0, 0, 0.15); |
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.
Use $border-fade-black
instead of the static value here.
&::before { | ||
top: -15px; | ||
margin-left: -9px; | ||
border: 8px solid transparent; |
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.
Use variables instead of static values here: $spacer-2 $border-style transparent
.
&::after { | ||
top: -13px; | ||
margin-left: -8px; | ||
border: 7px solid transparent; |
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.
Use $border-style
instead of solid
here.
modules/primer-popover/README.md
Outdated
@@ -0,0 +1,112 @@ | |||
# Primer CSS / Popover |
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.
Can you remove the /
we don't use that on the other readme's.
} | ||
|
||
&::before { | ||
top: -15px; |
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.
The border slightly overlaps into the Box area. I tweaked this in inspector (so let me know if I'm missing something) and changing ::before
to top: -16px
(or even better use $spacer-3
) and the ::after
to -14px
gets rid of the overlap. Because of the alpha transparency on the border you see slightly darker points where the border overlaps but it's barely visible and acceptable to me. Happy to explore a non alpha transparent border option for things like this though.
modules/primer-popover/README.md
Outdated
|
||
```html title="Center-aligned (default)" | ||
<div class="position-relative text-center"> | ||
<button class="btn btn-primary">UI</button> |
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.
Using a button here made me want to click on it to show/hide the popover because it kind of looks like a dropdown. Might be better to change this to a gray box with text along the lines of "New feature you want to draw attention to".
documentation for the ".Popover-message--bottom--right, .Popover-message--top--left, .Popover-message--bottom--left" selector(s)
@broccolini: Just pushed some changes that include the flexibility needed to accommodate github/github, and address some of your initial feedback. The big change here is that I've added support for modifiers to
Storybook demonstrates nearly all of these. The catalyst for this addition was the variety of locations within a layout where github/github uses Popovers, which called for the caret to be on the top edge of the popover, the left edge, and sometimes in different orientations along that edge. Rather than break their layout, I built in this flexibility so folks could more easily "design" the orientation of their Popover in what are often tight spaces. |
@broccolini: I addressed the caret pixels for ya. Good to go? |
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.
A couple of minor changes.
Biggest question I have is whether this should work with Box
, be a modifier of box (there are some similar modifiers in github/github
for Box--overlay
), or if we should build in the background, border, and box shadow into this component. As it is currently, we'd need to add primer-box
as a peer dependency since you can't fully create the component without.Box
.
Since this is how it's currently being used in dotcom I'm not worried about including that change in this pr (though we should add the PeerDependency), we can keep this as Experimental
status and v0.0.1 and follow up in a minor release.
top: -16px; | ||
margin-left: -9px; | ||
border: $spacer-2 $border-style transparent; | ||
border-bottom-color: rgba(0, 0, 0, 0.15); |
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.
Use $border-black-fade
instead of the static value.
|
||
&::before { | ||
bottom: -16px; | ||
border-top-color: rgba(0, 0, 0, 0.15); |
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.
Use -$spacer-3
and $border-black-fade
instead of the static values.
.Popover-message--right--bottom { | ||
&::before { | ||
right: -16px; | ||
border-left-color: rgba(0, 0, 0, 0.15); |
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.
Use -$spacer-3
and $border-black-fade
instead of the static values.
.Popover-message--left--bottom { | ||
&::before { | ||
left: -16px; | ||
border-right-color: rgba(0, 0, 0, 0.15); |
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.
Use -$spacer-3
and $border-black-fade
instead of the static values.
} | ||
} | ||
|
||
.Popover-message--lg { |
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.
How is this being used? In the docs it says it's a slightly larger option, but that's wrapped in a media query and not mentioned. If it's about the width and not the breakpoint, we should say --large
. Also I'd prefer this was divisible by 8 as well—does it work at 320px
?
} | ||
|
||
&::before { | ||
bottom: 20px; |
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.
Use $spacer-4
instead of the static value.
.Popover-message--left--top { | ||
&::before, | ||
&::after { | ||
top: 20px; |
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.
Use $spacer-4
instead of the static value.
|
||
&::before, | ||
&::after { | ||
left: 20px; |
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.
Use $spacer-4
instead of the static value.
|
||
.Popover-message { | ||
position: relative; | ||
width: 235px; |
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.
I'd like to make this an even number, suggest making this 232px
or 24px
—something divisible by 8px because that's what our spacing scale is based on. I know you based this on the original custom code, let me know if you this would cause issues, if so, we can adjust in a follow-up relelase.
Thanks @brandonrosage! 💖 I'm not going to approve until after we update the peer dependency update, which I will do either tonight or tomorrow morning. But otherwise I think this is good to go |
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 PR adds a new 'Popover' component, intended to suggest, guide, or bring attention to specific user interface elements on a page.
When present, it looks like this:
It's intended to establish a generalized, product-wide pattern from a convention that's used on github/github as a "TutorialPopover."
It's includes a small amount of unique CSS that adds a caret to a container (presumably one that uses the
.Box
component) and positions itself relative to a parent container. By default, it does so center-aligned. But with modifiers, it can also be left-aligned (.Popover-message--left
), right-aligned (.Popover-message--right
), and be assigned a larger width (.Popover-message--lg
).It assumes a context in which a relatively-positioned parent container includes:
It does not currently leverage any JavaScript to handle interaction with the nested button (e.g. "Got it!"), which would typically hide, remove, or dismiss the Popover.
/cc @primer/ds-core