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

Add media query hooks and animation to snackbar notices #15908

Merged
merged 4 commits into from Jun 3, 2019

Conversation

@youknowriad
Copy link
Contributor

commented May 30, 2019

This PR adds some animation to snackbar notices. It uses react-spring to do so and it introduces new React hooks: useMediaQuery and useReducedMotion in order to disable the animation if the reduced motion preference is enabled.

Testing instructions

  • Open the console
  • Add a notice
wp.data.dispatch('core/notices').createNotice(
		'info',
		'Post published.',
		{
			type: 'snackbar',
			actions: [
				{
					onClick: () => {},
					label: 'View post'
				}
			]
		}
	);
@gziolo

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Screen Shot 2019-05-30 at 11 41 08

How much of this size gets bundled in @wordpress/compose? Did you measure? After gzip, it might have very little impact, but it would be great to check it.

There are some e2e tests failing for metaboxes and classic editor. Do you think it might be related to animations added?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

I didn't add any dependency to compose so I don't expect any change in bundle size. react-spring is 9.7KB and that is going to be added to the components package.

@gziolo

This comment has been minimized.

Copy link
Member

commented May 30, 2019

I didn't add any dependency to compose so I don't expect any change in bundle size. react-spring is 9.7KB and that is going to be added to the components package.

Thanks for follow-up, it makes a lot of sense. I really like how @wordpress/compose is now structured to contain both hooks and HOCs 💯

@kjellr

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

This is excellent. The modal slides in from the bottom, and does a simple fade out on removal. Here's a (slightly slowed down) GIF of the animation:

animation

It respects the reduce motion media query too, and just appears/disappears in that case:

animation-reduce


The only issue I'm seeing occurs when there are multiple notices, and the bottommost ones are dismissed first:

animation-multiple

The notices near the top slide down as you'd expect, but they do a little pause before they settle into place at the very bottom. Is there anything we can do to eliminate that?

*
* @return {boolean} Reduced motion preference value.
*/
const useReducedMotion =

This comment has been minimized.

Copy link
@mtias

mtias May 30, 2019

Contributor

This should come in handy.

* @param {string} query Media Query.
* @return {boolean} return value of the media query.
*/
export default function useMediaQuery( query ) {

This comment has been minimized.

Copy link
@mtias

mtias May 30, 2019

Contributor

How would we handle these for mobile native conceptually?

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 31, 2019

Author Contributor

That's a good question, I think Mobile shouldn't really care about that hook. Ultimately, this hook might not be used directly by component. instead we'd use higher-level ones that can be "forked" by mobile for example useReducedMotion can be adapted for mobile.

await next( { height: 0 } );
},
config: { tension: 300 },
immediate: isReducedMotion,

This comment has been minimized.

Copy link
@mtias

mtias May 30, 2019

Contributor

I wonder if we should wrap around react-spring primitives and offer this by default.

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 31, 2019

Author Contributor

I'm not sure. Because there is not a unique hook for react spring animations so basically this means creating a custom hook for each one of the react-spring hooks.

@youknowriad youknowriad force-pushed the try/snackbar-animation branch from 4533fe1 to eb44d7f May 31, 2019

@youknowriad youknowriad requested review from nerrad, nosolosw and ntwb as code owners May 31, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

Pushed some updates here:

  • I introduced a new FORCE_REDUCED_MOTION to be able to force reduced motion for e2e test builds. I'd have preferred to avoid it and instead find a way to change the preference using a runtime code but it doesn't seem like there's a way to do it.

  • I fixed the "leave" animation (cc @kjellr), there's still a 1px jump at the end of the animation but I spent some time on it and I can't understand where it's coming from. I think it's not very noticeable enough to block this PR.

What do you all think? What's remaining here.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

there's still a 1px jump at the end of the animation but I spent some time on it and I can't understand where it's coming from

I'm unable to look right at this moment, but it sounds like the change from rendering on the GPU (which happens when you use translate) to rendering on the CPU (default state). This is usually mostly visible in Safari. If this is the case, keeping the item on the GPU forever should fix it, and can be done be applying will-change: transform; to the element that gets animated.

The only thing to be mindful of is that this can cause some trickiness when overflow is involved, and even then it's rare.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

@jasmussen Actually, I think it's if something happens when the DOM element containing the snackbar gets removed from the DOM. Before its removal, it is supposed to have a height of 0 which means removing it shouldn't have a visual impact but you do notice a very small jump.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

I'd appreciate reviews and if you think it's all good.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

I fixed the "leave" animation (cc @kjellr), there's still a 1px jump at the end of the animation but I spent some time on it and I can't understand where it's coming from. I think it's not very noticeable enough to block this PR.

@youknowriad — I think I found a quick fix to this one. Setting the tension back to the default 170 value (or removing that line entirely) removes this 1px jump. Not sure why, but it seems to work for me, and the animation still feels just fine. 🙂

I am seeing this PR crash GB in Safari — not sure if that's just a problem on my end. Here's the error I'm seeing.

@nerrad

nerrad approved these changes Jun 3, 2019

Copy link
Contributor

left a comment

I have just a few non-blocking comments (note this was a code review only, I did not test this).

<div
className="components-snackbar-list__notice-container"
ref={ ( ref ) => ref && refMap.set( notice, ref ) }
>

This comment has been minimized.

Copy link
@nerrad

nerrad Jun 3, 2019

Contributor

Why is this an additional nested div? Could the props and ref just be added to the <animated.div> component?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 3, 2019

Author Contributor

No we can't, that was my original ideal but the way this animation work required the animation divs to be adjacent without any margin between them (otherwise the animation is not smooth on removal, there's a jump) and it also requires that the height of the animated div is equal to the snackbar + the margin between two snackbars and without the inner div it's impossible to do because of the "margin-collapsing"


// Hooks
export { default as useMediaQuery } from './hooks/use-media-query';
export { default as useReducedMotion } from './hooks/use-reduced-motion';

This comment has been minimized.

Copy link
@nerrad

nerrad Jun 3, 2019

Contributor

Nice little bit of cleanup here 👍

@@ -44,6 +44,7 @@
"re-resizable": "^4.7.1",
"react-click-outside": "^3.0.0",
"react-dates": "^17.1.1",
"react-spring": "^8.0.20",

This comment has been minimized.

Copy link
@nerrad

nerrad Jun 3, 2019

Contributor

Is react-spring something we might consider adding as a library in WP core? I imagine a number of plugin/theme author's might use it as well (I'm currently using it for a block I'm developing).

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 3, 2019

Author Contributor

I personally worry about adding this kind of libs as WP core libs because of the breaking changes they might introduce. For the moment I think it's small enough to "allow it" to be duplicated but I think that's a good question and we should probably have some defined guidelines to answer it.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@kjellr Good find. Both are fixed now.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

Good find. Both are fixed now.

Excellent — just tested and confirmed. This should be ready to 🚢 from a design perspective!

@youknowriad youknowriad merged commit 392bf02 into master Jun 3, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad added this to the Gutenberg 5.9 milestone Jun 7, 2019

nicolad added a commit to nicolad/gutenberg that referenced this pull request Jun 15, 2019

@talldan talldan deleted the try/snackbar-animation branch Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.