Update Modal styling #31639
Merged
Update Modal styling #31639
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +117 B (0%) Total Size: 1.86 MB
ℹ️ View Unchanged
|
stokesman
force-pushed
the
update/modal-styling
branch
from
239bbc4 to
89e0634
Compare
stokesman
added
[Package] Components
stokesman
force-pushed
the
update/modal-styling
branch
from
89e0634 to
679a5a7
Compare
7 tasks
stokesman
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
stokesman
requested review from
jasmussen
and removed request for
ajitbohra,
chrisvanpatten and
ellatrix
jasmussen
approved these changes
May 25, 2021
There was a problem hiding this comment.
Solid work, thanks for tackling this. I left a few comments and have some here as well, but giving tentative approval as it a) solves issues, b) doesn't affect the tabbing order/accessibility.
It boils down to this. Before:
The subpixel glitching isn't visible here, but I've seen it in the past.
After:
Very nice.
A few small things:
This gets a bit big on big screens. We might want a max-width on this one, perhaps even a max-height. The rule could be keyboard-shortcut modal specific, and not a generalized constraint — I've no doubt some interfaces need a huge modal.
Same with block manager:
Preferences also feels a bit tall:
If those three can get a few max width/heights to look good on giant screens, this feels like a solid improvement. Thank you for your contribution!
| } | ||
| } | ||
|
|
||
| // Fix header to the top so it is always there to provide context to the modal | ||
| // if the content needs to be scrolled (for example, on the keyboard shortcuts | ||
| // modal screen). | ||
| .components-modal__header { | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
Is this removed because it wasn't needed or for some other reason?
There was a problem hiding this comment.
Thanks for asking about this as I'd meant to double-check it. It didn't seem needed as this will apply the same rule:
gutenberg/packages/edit-post/src/style.scss
Lines 69 to 72 in f10179b
But that could go wrong when Modal is used outside of WordPress packages like edit-post or edit-site. I've removed the width and set the right:0 to avoid that potential issue. Though, it makes me wonder why the reset to `.components-modal__frame' is applied in both those packages. Perhaps we should just apply it to the component itself?
There was a problem hiding this comment.
The reason I ask is that I think we should be using border-box very liberally, as in my experience it helps simplify all the adjacent CSS. If the effect is the same, fine to remove of course.
There was a problem hiding this comment.
That makes sense. I just can't help wanting to remove the redundancy. Since I've triple-checked this I realized that the modal header height may end up one pixel taller than intended if Modal is used standalone (outside of the edit-* packages that apply the reset to the modal). So I've put it back 😅 .
|
Thanks for the review. That was quite helpful. I had a rethink on the width of the modal frame element and now it preserves the existing widths of modals like the block manager and keyboard shortcuts. I've also added a generalized max-height that isn't very aggressive to the base modal and specific heights to the preferences modal. I think it goes quite well. I welcome any feedback (or commits of course). I tried to break up the new commits well to help make it easy to see the concerns they've addressed. |
|
This is still looking good, and the sizes look better:
Thanks again for the work! |
To allow easier sticky positioning for modal content elements and avoid modal sub-pixel positioning that can cause fuzzy rendering. - Let flex and auto margins provide centering instead of transform - Animate transform instead of margin for appear animation - Use absolute position instead of sticky for header - Remove redundant box-sizing rules - Cleanup of obviated rules
To work with the updated styling of the base Modal component and simplify the sticky position related styling. Some other style changes aimed to improve aesthetics.
stokesman
force-pushed
the
update/modal-styling
branch
from
403bf75 to
0acea32
Compare
|
Thanks again for the review! Had a failing test and then more failing tests on a rerun so I gave it a rebase and it went all green. Now I've pushed one last tiny commit to restore that box-sizing rule. |
|
Still looks good to me:
👌 |
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To allow easier sticky positioning for modal content elements and avoid modal sub-pixel positioning that can cause fuzzy rendering.
Primary changes
Motivation and Details
This began from wanting to help move #31191 along. Specifically to fix
position: stickystyling problem there. That immediate problem can be fixed with a simple change, but looking into it, it seemed more changes may be welcome to facilitate sticky positioning within modals and also fix their sometimes fuzzy rendering.Sticky fix and facilitation
Currently, the modal header is
stickyand can, in some cases, be scrolled out of view #31191 (comment). This PR fixes it by removing the header from the flow of the modal content. Doing so also simplifies positioning of other sticky elements within the modal. As it is now, to avoid overlap with the header, other sticky elements either have to be nested inside an additional overflowing context (as done in the Block Manager) or add the height of the header to theirtoppositions. Here are before/after demos of a custom modal with sticky elements styled withtop: 0:Snippet to run in console to add a sidebar with button to launch the modal seen above
The sometimes fuzzy rendering issue
If the modal has odd width or height values the
transform: translate(-50%, -50%)rule on the modal frame causes sub-pixel shifts resulting in gaps and fuzzy rendering of some children. These are likely best seen on a 1x resolution screen. Can be seen in some of the before screenshots to follow and in the following screen recording:before-rounding-errors.mp4
after-no-rounding-errors.mp4
Affected modals
I searched the codebase for implementors of
Modal, found and tested thirteen. Of those, four have changes in this PR that were needed due to the updatedModalstyles. Mostly the aim was to avoid any visual changes but a few deviations are made intentionally as proposed improvements.Block Compare Modal
This one had horizontal scrolling after the Modal style changes so it was updated to allow horizontal expansion. As a followup this one could probably use some small screen adjustments (preexistent issue).
Guide Component
Testing this with the Welcome Guide in the Post Editor and later in the Widgets Editor. It had a few issues after the Modal style changes but mostly just needed to override a different set of rules.
Block Manager Modal
This one mostly had padding/margin issues after the Modal style changes and I took some liberty in proposing some design changes. This modal is planned to be absorbed into the Preferences modal so I'm not sure if it requires too much scrutiny here.
Preferences Modal
This one overflowed vertically at slightly greater heights after the changes to Modal styling. I made a tweak for last-child sections to leave off their bottom margin. Kind of arbitrary, not necessary but seems safe and a slight improvement besides.
How has this been tested?
Types of changes
Bug fix
Code Quality
Breaking Change?: Though the changes are purely CSS they did affect the visual rendering of a few implementors in the codebase and could do the same for third parties. It seems very unlikely the change could create any sort of catastrophic breakage so I wouldn't think it truly classifies as breaking but I don't know if we have a policy around this.
Checklist:
*.native.jsfiles for terms that need renaming or removal).