-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Revise margins for notices in the notice list #16861
Conversation
haha, so I figured that the issue depens on whether you have an action or not :). With this change, the design is broken if you have an action, it's the opposite in master.
|
Good find. 😄 I messed around with this for a bit, and I actually still think the treatment in the PR is the best option for now. The The current close button is absolutely positioned. We could theoretically restyle this to use flexbox, and have the button center-aligned vertically, but that ends up looking weird when things wrap to more than one line: The current state of this PR how this situation was treated before #16589, so I think it at least reverts us back to that and leaves the door open for further improvements. That seems like a solid move for now. |
I think my main concern here is: Can we make sure that the height of the notice stays the same with or without action if it fits in one line? |
I pushed some updates to do that, but I feel that they're a little hacky. The issue is that the inline button pushes up the line height quite a bit. It notices were always on one line, we'd just add some extra line height to match, and we'd be all set. But we don't want to add too much line height, because when things wrap onto a second line, it'll look weird. That last commit seeks out a middle ground: taller (but still acceptable) line height than we had before, and a very slightly shorter button + button margins: |
Interesting, it makes me wonder why the actions are shown inline and not on the right. And align "text | actions | close" button using flex alignment or something? Happy to follow your judgment and go with the current approach though. |
Ha, I just realized I never pushed that change. 🙃 Anyway. I think we should:
(Here's a diff of the hack just for the record) Does that sound reasonable? |
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.
Works for me 👍
Looks like #16589 may have introduced a slight regression in the size/alignment of content inside of Notices that appear at the top of the screen. The notices appear too short, and the close button is misaligned. I'm not sure why this looked fine when we merged #16589, but doesn't work now:
In any case, this PR switches back to using
1em
margins on the top and bottom of.components-notice__content
to even that out:It only applies this to notices when they're inside of
.components-notice-list
, so it should have no negative effect on notices that appear inline, elsewhere. (Thus preserving the original intent of #16589).