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

Don't focus close button on modal open/Try a modal appear animation #9900

Merged
merged 4 commits into from
Oct 6, 2018
Merged

Don't focus close button on modal open/Try a modal appear animation #9900

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8f7bb55
Try a modal appear animation
jasmussen Sep 14, 2018
6d3b819
Explore a way to not show tooltip
jasmussen Sep 17, 2018
60f83a1
chore: Remove unneeded extra role and aria-label
tofumatt Oct 5, 2018
d1bf5ad
chore: Fix test
tofumatt Oct 6, 2018
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
14 changes: 14 additions & 0 deletions edit-post/assets/stylesheets/_animations.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,17 @@
@mixin animate_rotation($speed: 1s) {
animation: rotation $speed infinite linear;
}

@keyframes modal-appear {
from {
margin-top: $grid-size * 4;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Animating the margin is potentially not as performant as translate, but we can't use translate as we use that for responsive positioning at responsive breakpoints. @kjellr do you have any insights as to whether we can make this more performant? Can we use will-change: transform; yet? Or are we still using translateZ(0) for this?

Copy link
Contributor

@kjellr kjellr Sep 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I see what you mean — we're sort of backed into animating the margin here because of the other transform properties we're using. As far as I can tell, we could theoretically use translate if we declared separate, super-specific animations for each of the two breakpoints:

@keyframes modal-appear-small {
	from {
		transform: translate(-50%, 10%);
	}
	to {
		transform: translate(-50%, 0);
	}
}

@mixin modal_appear_small() {
	animation: modal-appear-small 0.1s ease-out;
	animation-fill-mode: forwards;
}

@keyframes modal-appear-medium {
	from {
		transform: translate(-50%, -20%);
	}
	to {
		transform: translate(-50%, -30%);
	}
}

@mixin modal_appear_medium() {
	animation: modal-appear-medium 0.1s ease-out;
	animation-fill-mode: forwards;
}

And then declarations like these in /modal/style.css:

@include break-small() {
	...

	// Animate appearance.
	@include modal_appear_small();
}
@include break-medium () {
	...

	// Animate appearance.
	@include modal_appear_medium();
}

... but that feels like a bit of a hack (and a longwinded one at that), and I'm not 100% sure that it gains us a lot in terms of performance. So maybe we just stick to animating the margin?

Regarding will-change: I'm not totally familiar with that property, but I'm getting mixed signals about its usefulness when reading the MDN page. Browser support aside, they list a lot of caveats — based on what I'm reading there, I'm not sure an animation this small actually merits the use of it.

}
to {
margin-top: 0;
}
}

@mixin modal_appear() {
animation: modal-appear 0.1s ease-out;
animation-fill-mode: forwards;
}
9 changes: 0 additions & 9 deletions edit-post/assets/stylesheets/main.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,12 +278,3 @@ body.gutenberg-editor-page {
.ephox-snooker-resizer-bar-dragging {
background: $blue-medium-500;
}

// This style is defined here instead of the modal component
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'm not exactly sure what this means — the height of the modal is defined at responsive breakpoints already, this did not seem to account for that. But regardless of that, I may have been able to refactor this out by removing the need for calc here. Can you take a look, @youknowriad?

Copy link
Contributor

Choose a reason for hiding this comment

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

It means we shouldn't assume we're in the context of the WP Admin page when tweaking the style of the modal in the style.scss of the modal component. Which means we can't use things like:

  • $admin-bar-height
  • body.is-fullscreen-mode

And as far as I can tell, this PR rewrites the modal component to be shown in the same way regardless of the pagee it's used in (editor or not), so I think we're probably fine removing those.

// because the modal should be context agnostic.
.components-modal__content {
height: calc(100% - #{ $header-height } - #{ $admin-bar-height });
body.is-fullscreen-mode & {
height: calc(100% - #{ $header-height });
}
}
29 changes: 18 additions & 11 deletions packages/components/src/modal/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { noop } from 'lodash';
*/
import { Component, createPortal } from '@wordpress/element';
import { withInstanceId } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
Expand Down Expand Up @@ -135,7 +136,8 @@ class Modal extends Component {
'components-modal-header-' + instanceId
);

// Disable reason: this stops mouse events from triggering tooltips and other elements underneath the modal overlay
// Disable reason: this stops mouse events from triggering tooltips and
// other elements underneath the modal overlay.
/* eslint-disable jsx-a11y/no-static-element-interactions */
return createPortal(
<div
Expand All @@ -152,17 +154,22 @@ class Modal extends Component {
labelledby: title ? headingId : null,
describedby: aria.describedby,
} }
{ ...otherProps } >
<ModalHeader
closeLabel={ closeButtonLabel }
isDismissable={ isDismissable }
onClose={ onRequestClose }
title={ title }
headingId={ headingId }
icon={ icon }
/>
{ ...otherProps }
>
<div
className={ 'components-modal__content' }>
className={ 'components-modal__content' }
role="region"
aria-label={ __( 'Dialog Contents' ) }
tabIndex="0"
>
<ModalHeader
closeLabel={ closeButtonLabel }
headingId={ headingId }
icon={ icon }
isDismissable={ isDismissable }
onClose={ onRequestClose }
title={ title }
/>
{ children }
</div>
</ModalFrame>
Expand Down
78 changes: 50 additions & 28 deletions packages/components/src/modal/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,18 @@
left: 0;
background-color: rgba($white, 0.4);
z-index: z-index(".components-modal__screen-overlay");

// Animate appearance.
@include fade-in();
}

// The modal window element.
.components-modal__frame {
border: $border-width solid $light-gray-500;
background-color: $white;
box-shadow: $shadow-modal;
outline: none;

// In small screens the content needs to be full width.
position: fixed;
top: 0;
Expand All @@ -30,6 +38,9 @@
top: $panel-padding;
left: 50%;
height: 90%;

// Animate appearance.
@include modal_appear();
}

// Show pretty big on desktop breakpoints.
Expand All @@ -40,50 +51,61 @@
left: 50%;
height: 70%;
}

border: $border-width solid $light-gray-500;
background-color: $white;
box-shadow: $shadow-modal;
outline: none;
}

// Fix heading to the top.
.components-modal__header {
box-sizing: border-box;
height: $header-height;
border-bottom: $border-width solid $light-gray-500;
padding: $item-spacing $item-spacing $item-spacing $panel-padding;
padding: 0 0 $grid-size 0;
display: flex;
flex-direction: row;
align-items: stretch;
justify-content: space-between;
}
position: fixed;
top: 0;
background: $white;
width: calc(100% - #{$panel-padding + $panel-padding});
height: $header-height;
padding: $item-spacing 0;

.components-modal__header-heading-container {
align-items: center;
flex-grow: 1;
display: flex;
flex-direction: row;
justify-content: left;
}
.components-modal__header-heading {
font-size: 1em;
font-weight: normal;
}

.components-modal__header-heading {
font-size: 1em;
font-weight: normal;
}
.components-modal__header-heading-container {
align-items: center;
flex-grow: 1;
display: flex;
flex-direction: row;
justify-content: left;
}

.components-modal__header-icon-container {
display: inline-block;
.components-modal__header-icon-container {
display: inline-block;

svg {
max-width: $icon-button-size;
max-height: $icon-button-size;
padding: 8px;
svg {
max-width: $icon-button-size;
max-height: $icon-button-size;
padding: 8px;
}
}

h1 {
line-height: 1;
margin: 0;
}
}

// Modal contents.
.components-modal__content {
// The height of the content is the height of it's parent, minus the header. after that, the offset was 3px.
height: 100%;
box-sizing: border-box;
overflow: auto;
padding: $panel-padding;
height: 100%;
padding: ($header-height+$panel-padding) $panel-padding $panel-padding $panel-padding;

&:focus {
outline: $border-width solid $dark-gray-500;
}
}