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

feat: add utility for menu styles #655

Merged
merged 18 commits into from
Mar 2, 2020
Merged

feat: add utility for menu styles #655

merged 18 commits into from
Mar 2, 2020

Conversation

jzempel
Copy link
Member

@jzempel jzempel commented Feb 29, 2020

Description

With the new menuStyles utility, menus in Garden are now clearly:

  1. A positioning wrapper (facilitated via Popper)
  2. A styled menu child

Features

  • New menuStyles utility added to react-theming and shared between react-datepickers and react-dropdowns
  • Ability to target menu wrappers with themed component custom styling

Fixes

  • Added visibility controls (via useEffect with timeouts) to restore fade-out animation for both datepicker and dropdown menus –  providing subtle, important feedback that a user's action was effective
  • Implemented CSS that cut out unnecessary "div soup" in dropdown menus.
    • Hiding arrow border during first phase of animation got rid of StyledMenuAnimation div.
    • Overriding menu positioning to static allows arrow to display beyond overflow-y and got rid of StyledMaxHeightWrapper div.
  • Restored semantic <ul> - <li> menu structure in dropdowns.

Other

  • Document no-JSX expectation for view components. This cleans up code clutter and, in this PR, improved bundle sizing.

Detail

Demo site is pre-published for review. Notable links include:

  • theming with knob controls to demonstrate the new menuStyles functionality
  • datepickers updated to ensure the calendar dismiss fades out as expected
  • dropdowns updated to ensure the menu dismiss fades out as expected

Merge of this PR will be paired with #651 to publish react-components v8.1.

Checklist

  • 👌 design updates are Garden Designer approved (add the
    designer as a reviewer)
  • 🌐 Styleguidist demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • ♿ analyzed via axe and evaluated using VoiceOver
  • 💂‍♂️ includes new unit tests
  • 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

@coveralls
Copy link

coveralls commented Feb 29, 2020

Coverage Status

Coverage decreased (-0.1%) to 98.067% when pulling db1cf6c on jzempel/menu-styles into 893194b on master.

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.

Left some comments, but overall this looks good.

We will also need to bump the peerDependency versions for react-theming to ^8.1.0 in all packages. I can do that in #651

const animationName = keyframes`
0%, 66% {
${property}: 2px;
border: transparent; /* [1] */
Copy link
Contributor

Choose a reason for hiding this comment

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

So this was the trick when I first moved over to transform: translate for the animation. Good find.

packages/dropdowns/src/elements/Menu/Menu.tsx Outdated Show resolved Hide resolved
packages/datepickers/src/styled/StyledMenu.ts Outdated Show resolved Hide resolved
@jzempel jzempel merged commit 99d33d1 into master Mar 2, 2020
@jzempel jzempel deleted the jzempel/menu-styles branch March 2, 2020 17:54
Copy link

@ginnywood ginnywood 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 awesome, the animation looks great. Noticed a couple weird things:

  1. In the Tree Menu usage, if I select an item from the nested menu, I see a flash of the parent menu as it fades out. https://share.getcloudapp.com/rRu67RyB
  2. In the Multiselect, selecting an item adds it as a tag and text. This happens on garden.zendesk.com too, so it’s outside the scope of this branch, but I thought I’d flag it. https://share.getcloudapp.com/JruW6BX9

@jzempel
Copy link
Member Author

jzempel commented Mar 2, 2020

Nice finds @ginnywood. I'm going to add an issue for us to treat these as follow-on bugs 🐛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants