Skip to content

Conversation

@ryanseddon
Copy link
Contributor

@ryanseddon ryanseddon commented Dec 14, 2018

Description

This PR is the first step to allow support for top and left animations to work correctly with popper.js in our select & menu components.

Detail

The real change is just using top, left, right & bottom instead of margins.

Popper positions the element using a transform so top, left, right & bottom are relative to that gpu layer and not to the document.

menus-anim

react-menus

Here's this running locally for react-menus

react-menus-anim

Less important stuff

I added a .prettierrc file that matches react-components.

The demo page needed some changes to support working with the new usage of position properties over using margins to emulate what popper would do.

Checklist

  • 👌 style updates are Garden Designer approved (add the
    designer as a reviewer)
  • 🌐 component demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

@ryanseddon plz review https://github.com/zendeskgarden/css-menus/pull/20 which explains the rationale for using margin to animate and ensure that your changes don't regress the behavior. Also, please grep for c-menu usage throughout the demo pages and check that everything is still working as expected.

Copy link
Contributor

@austingreendev austingreendev left a comment

Choose a reason for hiding this comment

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

Approved pending research into the issues that @jzempel pointed out.

@ryanseddon
Copy link
Contributor Author

@jzempel yep this bug has not regressed tested in all supported browsers and it looks good, will grep and test usages throughout demo pages now.

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

@ryanseddon am I reading this right that now a container is needed in order to support the animation? If so, do you consider this to be a major breaking change?

@ryanseddon
Copy link
Contributor Author

@jzempel not really, if someone was going to just use the css of this they'd have to build the logic themselves to correctly position the menu view.

This container matches what popper.js does already in our react-component so it's not a breaking change in react.

@ryanseddon ryanseddon merged commit f433307 into master Dec 21, 2018
@ryanseddon ryanseddon deleted the ryan/fix_animations branch December 21, 2018 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants