Skip to content

Conversation

@dfmcphee
Copy link
Contributor

@dfmcphee dfmcphee commented Nov 8, 2021

WHY are these changes introduced?

This PR makes a few improvements to the Popover component including some foundational changes in things that it uses.

WHAT is this pull request doing?

  1. It improves the animation to feel snappier. It has been updated to use the fast duration and an updated base easing curve woking with @johanstromqvist. It also removed the animation on close to make it get out of the users way immediately.

Before

popover-motion-before.mov

After

popover-motion-after.mov
  1. Bumps our default max-height for popovers to avoid forcing users to scroll the popover when they shouldn't need to.

Before

Screen Shot 2021-11-09 at 11 01 40 AM

After

Screen Shot 2021-11-09 at 11 01 28 AM

  1. Remove the scroll hint by default on popover panes. Currently we animate any scrollable popover on open, which can be disorienting. By increasing the max-height this happens less often, but we also removed this as the default option.

Before

popover-scroll-before.mov

After

popover-scroll-after.mov
  1. Fixes a bug where popover or tooltip contents could be hidden behind the top bar if set to preferredPosition="above".

Before

Screen Shot 2021-11-09 at 10 51 34 AM

After

Screen Shot 2021-11-09 at 10 51 13 AM

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2021

size-limit report

Path Size
cjs 166.08 KB (+0.02% 🔺)
esm 96.63 KB (+0.04% 🔺)
esnext 143.5 KB (+0.03% 🔺)
css 34.53 KB (+0.02% 🔺)

@dfmcphee dfmcphee force-pushed the popover-motion-improvements branch from af7d85c to 805026a Compare November 9, 2021 16:07
@dfmcphee dfmcphee marked this pull request as ready for review November 9, 2021 16:07
@ouellettejordan
Copy link

🚢

@dfmcphee dfmcphee requested review from alex-page and dleroux November 9, 2021 16:12
Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

So good 🤌🏽

Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

This is great. Thanks @dfmcphee! I am only unsure about the will-change property and if it is actually needed for the transform and opacity. Happy to merge but would love a quick investigation to see if it is needed.

Don’t apply will-change to elements to perform premature optimization. If your page is performing well, don’t add the will-change property to elements just to wring out a little more speed. will-change is intended to be used as something of a last resort, in order to try to deal with existing performance problems. It should not be used to anticipate performance problems. Excessive use of will-change will result in excessive memory use and will cause more complex rendering to occur as the browser attempts to prepare for the possible change. This will lead to worse performance.
https://developer.mozilla.org/en-US/docs/Web/CSS/will-change

@johanstromqvist
Copy link
Contributor

Great stuff @ouellettejordan and @dfmcphee!

The asymmetric enter/exit patterns wasn't mentioned, i.e. removing exit transition. Perhaps not essential to the PR? A strong default and scalable pattern for sure!

  • Enter: fast + reinforce relationship
  • Exit: immediately gone

@dfmcphee
Copy link
Contributor Author

dfmcphee commented Nov 9, 2021

I am only unsure about the will-change property and if it is actually needed for the transform and opacity. Happy to merge but would love a quick investigation to see if it is needed.

Comparing with and without the will-change the performance is almost identical. I am going to remove it.

The asymmetric enter/exit patterns wasn't mentioned, i.e. removing exit transition. Perhaps not essential to the PR? A strong default and scalable pattern for sure!

Good catch. I will update the description to reflect this too.

@dleroux
Copy link
Contributor

dleroux commented Nov 9, 2021

Awesome! Turned out to be a lot simpler than I was thinking.

@dfmcphee dfmcphee merged commit f89f658 into main Nov 9, 2021
@dfmcphee dfmcphee deleted the popover-motion-improvements branch November 9, 2021 18:01
@ahmed-adly-khalil
Copy link

@dfmcphee Thank you so much for this PR, this was very much needed, especially the scroll part

@emma-boardman emma-boardman restored the popover-motion-improvements branch February 14, 2022 15:22
@emma-boardman emma-boardman deleted the popover-motion-improvements branch February 14, 2022 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants