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

Update the modal design #40781

Merged
merged 23 commits into from Jul 6, 2022
Merged
Show file tree
Hide file tree
Changes from all 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -175,7 +175,7 @@ const BlockPatternSetup = ( {
activeSlide={ activeSlide }
patterns={ patterns }
onBlockPatternSelect={ onPatternSelectCallback }
height={ contentHeight - 2 * 60 }
height={ contentHeight - ( 74 + 60 ) } // Content - ( modal header + modal actions )
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding an inline comment to explain how the height is computed here!

I wonder if there's any way we could avoid using these hardcoded numbers altogether and use either variables, or a layout that intrinsically adopts the correct height.

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 agree. Perhaps we can look into that when we address the border issue in this particular modal.

/>
<SetupToolbar
viewMode={ viewMode }
Expand Down
Expand Up @@ -17,6 +17,7 @@
display: block;
width: 100%;
padding: $grid-unit-40;
padding-top: 0;
column-count: 2;

@include break-huge() {
Expand Down Expand Up @@ -54,7 +55,6 @@
margin: 0;
color: $gray-900;
// Block UI appearance.
border-radius: $radius-block-ui $radius-block-ui 0 0;
background-color: $white;
display: flex;
flex-direction: row;
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/inserter/style.scss
Expand Up @@ -342,7 +342,7 @@ $block-inserter-tabs-height: 44px;
.block-editor-block-patterns-explorer {
&__sidebar {
position: absolute;
top: $header-height;
top: $header-height + $grid-unit-20;
left: 0;
bottom: 0;
width: $sidebar-width;
Expand Down
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Expand Up @@ -167,6 +167,7 @@

- `InputControl`: Add `__next36pxDefaultSize` flag for larger default size ([#40622](https://github.com/WordPress/gutenberg/pull/40622)).
- `UnitControl`: Add `__next36pxDefaultSize` flag for larger default size ([#40627](https://github.com/WordPress/gutenberg/pull/40627)).
- `Modal` design adjustments: Blur elements outside of the modal, increase modal title size, use larger close icon, remove header border when modal contents are scrolled. ([#40781](https://github.com/WordPress/gutenberg/pull/40781)).
- `SelectControl`: Improved TypeScript support ([#40737](https://github.com/WordPress/gutenberg/pull/40737)).
- `ToggleControlGroup`: Switch to internal `Icon` component for dashicon support ([40717](https://github.com/WordPress/gutenberg/pull/40717)).
- Improve `ToolsPanel` accessibility. ([#40716](https://github.com/WordPress/gutenberg/pull/40716))
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/guide/style.scss
Expand Up @@ -20,6 +20,7 @@
border-bottom: none;
padding: 0;
position: sticky;
height: $header-height;

.components-button {
align-self: flex-start;
Expand Down
23 changes: 21 additions & 2 deletions packages/components/src/modal/index.js
Expand Up @@ -10,8 +10,10 @@ import classnames from 'classnames';
*/
import {
createPortal,
useCallback,
useEffect,
useRef,
useState,
forwardRef,
} from '@wordpress/element';
import {
Expand All @@ -24,7 +26,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 @@ -73,6 +75,8 @@ function Modal( props, forwardedRef ) {
const focusReturnRef = useFocusReturn();
const focusOutsideProps = useFocusOutside( onRequestClose );

const [ hasScrolledContent, setHasScrolledContent ] = useState( false );

useEffect( () => {
openModalCount++;

Expand Down Expand Up @@ -104,6 +108,19 @@ function Modal( props, forwardedRef ) {
}
}

const onContentContainerScroll = useCallback(
( e ) => {
const scrollY = e?.target?.scrollTop ?? -1;

if ( ! hasScrolledContent && scrollY > 0 ) {
setHasScrolledContent( true );
} else if ( hasScrolledContent && scrollY <= 0 ) {
setHasScrolledContent( false );
}
},
[ hasScrolledContent ]
);

return createPortal(
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
<div
Expand Down Expand Up @@ -142,8 +159,10 @@ function Modal( props, forwardedRef ) {
<div
className={ classnames( 'components-modal__content', {
'hide-header': __experimentalHideHeader,
'has-scrolled-content': hasScrolledContent,
} ) }
role="document"
onScroll={ onContentContainerScroll }
>
{ ! __experimentalHideHeader && (
<div className="components-modal__header">
Expand All @@ -168,7 +187,7 @@ function Modal( props, forwardedRef ) {
{ isDismissible && (
<Button
onClick={ onRequestClose }
icon={ closeSmall }
icon={ close }
label={
closeButtonLabel ||
__( 'Close dialog' )
Expand Down
18 changes: 17 additions & 1 deletion packages/components/src/modal/stories/index.js
Expand Up @@ -35,7 +35,23 @@ const ModalExample = ( props ) => {
Open Modal
</Button>
{ isOpen && (
<Modal { ...props } onRequestClose={ closeModal }>
<Modal
{ ...props }
onRequestClose={ closeModal }
style={ { maxWidth: '600px' } }
>
<p>
Lorem ipsum dolor sit amet, consectetur adipiscing elit,
sed do eiusmod tempor incididunt ut labore et magna
aliqua. Ut enim ad minim veniam, quis nostrud
exercitation ullamco laboris nisi ut aliquip ex ea ea
commodo consequat. Duis aute irure dolor in
reprehenderit in voluptate velit esse cillum dolore eu
fugiat nulla pariatur. Excepteur sint occaecat cupidatat
non proident, sunt in culpa qui officia deserunt mollit
anim id est laborum.
</p>

<Button variant="secondary" onClick={ closeModal }>
Close Modal
</Button>
Expand Down
27 changes: 9 additions & 18 deletions packages/components/src/modal/style.scss
Expand Up @@ -8,6 +8,7 @@
background-color: rgba($black, 0.35);
z-index: z-index(".components-modal__screen-overlay");
display: flex;
backdrop-filter: blur($grid-unit);

// This animates the appearance of the white background.
@include edit-post__fade-in-animation();
Expand Down Expand Up @@ -63,21 +64,21 @@
// modal screen).
.components-modal__header {
box-sizing: border-box;
border-bottom: $border-width solid $gray-300;
border-bottom: $border-width solid transparent;
padding: 0 $grid-unit-40;
display: flex;
flex-direction: row;
justify-content: space-between;
align-items: center;
height: $header-height;
height: $header-height + $grid-unit-20;
width: 100%;
z-index: z-index(".components-modal__header");
position: absolute;
top: 0;
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 All @@ -90,6 +91,10 @@
position: relative;
left: $grid-unit-10;
}

.components-modal__content.has-scrolled-content:not(.hide-header) & {
border-bottom-color: $gray-300;
}
}

.components-modal__header-heading-container {
Expand All @@ -113,26 +118,12 @@
// 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

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

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;

// Emulate margin-bottom for the header. Uses a pseudo-element since the
// absolutely positioned header’s margins wouldn’t effect siblings and
// padding-top on the content element would effect positioning of any
// sticky elements within.
&::before {
content: "";
display: block;
margin-bottom: $grid-unit-30;
}

&.hide-header {
margin-top: 0;
padding-top: $grid-unit-30;

&::before {
content: none;
}
}
}
Expand Up @@ -3,11 +3,6 @@
margin: 0 0 2rem 0;
}

&__main-shortcuts .edit-post-keyboard-shortcut-help-modal__shortcut-list {
// Push the shortcut to be flush with top modal header.
margin-top: -$grid-unit-30 -$border-width;
}

&__section-title {
font-size: 0.9rem;
font-weight: 600;
Expand Down
10 changes: 0 additions & 10 deletions packages/edit-post/src/components/sidebar/post-template/style.scss
Expand Up @@ -29,16 +29,6 @@
margin: $grid-unit-10;
}

.edit-post-post-template__create-modal {
.components-modal__header {
border-bottom: none;
}

.components-modal__content::before {
margin-bottom: $grid-unit-05;
}
}

.edit-post-post-template__create-form {
@include break-medium() {
width: $grid-unit * 40;
Expand Down
35 changes: 35 additions & 0 deletions packages/edit-post/src/components/sidebar/template/style.scss
@@ -0,0 +1,35 @@
.edit-post-template__modal {
.components-base-control {
@include break-medium() {
width: $grid-unit * 40;
}
}
}

.edit-post-template__modal-actions {
margin-top: $grid-unit-15;
}

.edit-post-template-modal__tip {
padding: $grid-unit-20 $grid-unit-30;
background: $gray-100;
border-radius: $radius-block-ui;

@include break-medium() {
width: $grid-unit * 30;
}
}

.edit-post-template__notice {
margin: 0 0 $grid-unit-10 0;

.components-notice__content {
margin: 0;
}
}

.edit-post-template__actions {
button:not(:last-child) {
margin-right: $grid-unit-10;
}
}
Expand Up @@ -11,15 +11,6 @@
@include break-large() {
height: 70%;
}

// @todo: Consider this for a minimal modal prop.
.components-modal__header {
border-bottom: none;
}

.components-modal__content::before {
content: none;
}
}

// 2 column masonry layout.
Expand Down
Expand Up @@ -3,11 +3,6 @@
margin: 0 0 2rem 0;
}

&__main-shortcuts .edit-site-keyboard-shortcut-help-modal__shortcut-list {
// Push the shortcut to be flush with top modal header.
margin-top: -$grid-unit-30 -$border-width;
}

&__section-title {
font-size: 0.9rem;
font-weight: 600;
Expand Down
8 changes: 0 additions & 8 deletions packages/edit-site/src/components/list/style.scss
Expand Up @@ -131,14 +131,6 @@
width: $grid-unit * 40;
}
}

.components-modal__header {
border-bottom: none;
}

.components-modal__content::before {
margin-bottom: $grid-unit-05;
}
}

.edit-site-list__rename-modal-actions {
Expand Down
Expand Up @@ -16,10 +16,6 @@
@media (max-width: #{ ($break-medium - 1) }) {
.components-modal__content {
padding: 0;

&::before {
content: none;
}
}
}
}