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.

Sign up for GitHub

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

Jump to bottom

Update the modal design #40781

Merged
merged 23 commits into from
Jul 6, 2022
Merged

Update the modal design #40781

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
aa4fecb
blur content behind modal
jameskoster Apr 29, 2022
dc51eac
modal styling
jameskoster Apr 29, 2022
e1975a4
Revert radii
jameskoster May 4, 2022
f87325a
Update components changelog
jameskoster May 9, 2022
8239136
Use backdrop-filter rather than filter.
jameskoster May 9, 2022
d970066
Merge branch 'trunk' into update/modal-UI
jameskoster May 9, 2022
7d3cb99
Add more content to modal in Storybook
ciampo May 25, 2022
f280207
Show border between header and content only if content has scrolled
ciampo May 25, 2022
b7aa9d2
Apply suggestions from code review
ciampo May 25, 2022
a746fe3
Adjust header height and padding
jameskoster May 25, 2022
b3123dc
Simplify scroll ev listener implementation
ciampo May 27, 2022
88c3f11
fix lint errors
ciampo May 27, 2022
07a69b6
Remove pseudo margin
jameskoster May 31, 2022
90db6fe
Query pattern selector styling
jameskoster May 31, 2022
a4cc018
keyboard shortcuts in the site editor
jameskoster May 31, 2022
faeb082
Set blur value to 2px
jameskoster Jun 7, 2022
756897b
Update packages/components/CHANGELOG.md
jameskoster Jun 9, 2022
2a9bec5
Revert "Set blur value to 2px"
jameskoster Jun 10, 2022
f59f3ca
Merge branch 'trunk' into update/modal-UI
jameskoster Jun 10, 2022
9000b7c
Remove un-needed styles
jameskoster Jul 4, 2022
8bfcc12
Merge remote-tracking branch 'origin/trunk' into update/modal-UI
noisysocks Jul 5, 2022
d1dbfba
Adjust position of filter panel in pattern explorer
jameskoster Jul 6, 2022
b5c595c
Merge branch 'trunk' into update/modal-UI
jameskoster Jul 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/components/src/modal/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
} from '@wordpress/compose';
import { ESCAPE } from '@wordpress/keycodes';
import { __ } from '@wordpress/i18n';
import { closeSmall } from '@wordpress/icons';
import { close } from '@wordpress/icons';

/**
* Internal dependencies
Expand Down Expand Up @@ -168,7 +168,7 @@ function Modal( props, forwardedRef ) {
{ isDismissible && (
<Button
onClick={ onRequestClose }
icon={ closeSmall }
icon={ close }
label={
closeButtonLabel ||
__( 'Close dialog' )
Expand Down
15 changes: 10 additions & 5 deletions packages/components/src/modal/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
body.modal-open {
#wpwrap {
filter: blur($grid-unit);
}
}

// The scrim behind the modal window.
.components-modal__screen-overlay {
position: fixed;
Expand All @@ -20,7 +26,7 @@
width: 100%;
background: $white;
box-shadow: $shadow-modal;
border-radius: $radius-block-ui;
border-radius: $grid-unit;
overflow: hidden;
// Have the content element fill the vertical space yet not overflow.
display: flex;
Expand Down Expand Up @@ -63,8 +69,7 @@
// modal screen).
.components-modal__header {
box-sizing: border-box;
border-bottom: $border-width solid $gray-300;
padding: 0 $grid-unit-40;
padding: $grid-unit-20 $grid-unit-40 0;
display: flex;
flex-direction: row;
justify-content: space-between;
Expand All @@ -77,7 +82,7 @@
left: 0;

.components-modal__header-heading {
font-size: 1rem;
font-size: 1.2rem;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice for things like this to be systematised, I'm aware of the experimental Text component, but it's not widely used. Perhaps there's a better way?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd personally love to see the Text and Heading components user more often.

I'm not sure if we need to first assess the type scale (and the rest of the styles), but otherwise I think we should slowly try to adopt them in other components, find out if they match our needs and make any necessary changes, before then rolling them out more widely across Gutenberg

font-weight: 600;
}

Expand Down Expand Up @@ -113,7 +118,7 @@
// Modal contents.
.components-modal__content {
flex: 1;
margin-top: $header-height;
margin-top: $header-height + $grid-unit-20;
Copy link
Contributor

@ciampo ciampo Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just flagging that this change may have introduced other layout regressions in places where it was assumed that the Modal's header was $header-height px tall.

One such example is the block pattern inserter/explorer modal, where in my testing I spotted a regression caused by the fact that the sidebar was positioned at $header-height px from the top of its container

gutenberg/packages/block-editor/src/components/inserter/style.scss

Line 345 in 8b14921

top: $header-height;

image

In general, consumers of the Modal component should not assume implementation details like the height of its header — when that happens, a small change like this PR is enough to cause a regression (which is also what I was trying to flag in this other comment of mine ).

While a short-term fix would be to update the line of code linked above to use the updated Modal's header height, I would argue that the best way to fix this specific regression in the block pattern inserter / explorer is to rewrite the modal's internal layout in a way that avoids absolute positioning and hardcoded margins/positions.

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 would argue that the best way to fix this specific regression in the block pattern inserter / explorer is to rewrite the modal's internal layout avoiding absolute positioning and hardcoded distances.

I agree, the hardcoded height also causes issues where the modal title is long enough to wrap (uncommon but not impossible). Do you think we should do this as a part of this PR or elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the best course of action is to:

  • add a quick fix in this PR (should be as easy as changing from top: $header-height; to top: $header-height + $grid-unit-20;
  • open a few follow-up PRs to:
    • refactor the layout of the component as suggested before.
    • try to refactor away from the hardcoded height as discussed in this other comment

padding: 0 $grid-unit-40 $grid-unit-30;
overflow: auto;

Expand Down