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

[Emotion] Convert EuiButton #6150

Merged
merged 4 commits into from
Aug 23, 2022
Merged

[Emotion] Convert EuiButton #6150

merged 4 commits into from
Aug 23, 2022

Conversation

miukimiu
Copy link
Contributor

This PR is basically a squash of all the commits from #5989. When I merged main and because Caroline is no longer working at Elastic it was necessary to sign the contribution agreement. Because she did all the commits when she was still an Elastic employee I don't see an issue on squash all the commits into one.

1. Moved Amsterdam-specific Sass overrides to default global_styling/ folder

This just cleans up some of the unnecessary overrides and Sass files. But mostly leaves the variable / mixins in tact for now.

2. New theme-specific (Amsterdam only for now) button styling functions

  • Removed disabled as a specific BUTTON_COLOR but allow it to be passed into euiButtonColor for coloring
  • euiButtonColor() now returns both backgroundColor and color for proper text contrast
  • euiButtonFillColor() creates the fill (solid color, white/black text) styling combo
  • euiButtonEmptyColor() creates the empty style using text-variants for color and transparent hover backgrounds
  • useEuiButtonColorCSS() now accepts options such as the display: 'base' | 'fill' | 'empty' and returns both keys for color and display.
  • useEuiButtonRadiusCSS() returns the border-radius values by size of button
  • useEuiButtonFocusCSS() is just the translate animation

3. Button component updates

EuiButton

  • Renamed exported ButtonColor to EuiButtonColor and ButtonSize to EuiButtonSize
  • Using the new EuiButtonDisplay component as the render and passing on all the rest of the props to handle the button vs anchor logic
  • Using Emotion for color, fill, and disabled and removing associated classes.
  • Fixed outline focus color of text color fill buttons
  • Background colors for base buttons are no longer transparent

The new tinted colors are just slightly different from their old transparent counterpart. The biggest difference being in the text version which will now align better to forms.

image

Note
This basically means this whole component is now Emotion since EuiButton now only handles superficial styling, everything else is passed through to the already Emotion EuiButtonDisplay.

EuiButtonEmpty

  • Using Emotion for color, and disabled and removing associated classes.

Note
There's still more we can do here to make use of EuiButtonDisplay but I didn't want to get too far into the weeds.

EuiButtonIcon

  • Using Emotion for color, disabled, and display removing associated classes.

Note
There's still more we can do here to make use of EuiButtonDisplay but I didn't want to get too far into the weeds.

EuiButtonGroup & Option

  • [Breaking]: Removed support for ghost. The combo styles between Emotion and CSS were too complex to maintain, but it felt safe to remove support (based an searching for usages - none) and will guide consumers to use colorMode instead once fully converted.
  • Using Emotion for color, disabled, and isSelected (as display='fill')

Note
This still uses the old/deprecated EuiButtonDisplay for now

EuiButtonDisplay

Since this is the component that renders the actual element (button vs anchor) I have moved all the logic around the associated props of each from EuiButton to this file so all consumers of this can benefit / doesn't have to worry about the actual DOM element.

EuiButtonDisplayContent

I moved the currently applied disabled styles to be handled by EuiButtonDisplay instead while also applying truncation to the text span using the utility class.

Other updates

  • Added isButtonDisabled() utility for DRY-ing out the logic which compares the disabled, isDisabled, isLoading, and validity of href to determine final disabled state.
  • [EuiMarkdownEditorToolbar] Added data-test-subj props to empty buttons for tests

Checklist

  • BWC for ghost colors
  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • [ ] Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6150/

@miukimiu miukimiu marked this pull request as ready for review August 22, 2022 13:58
@miukimiu
Copy link
Contributor Author

@thompsongl just giving you some context. I approved #5989 and it was just waiting for your review.

This PR is basically the same PR (#5989) but I squashed the commits because there were some issues when I merged main. Basically, the CL Checker needed Caroline to sign the Contributor Agreement. But the commits were done while Caroline was still working at Elastic. So I just squashed all the commits into a new branch. Let me know if that's ok.

Also, there was an issue that I found in that PR but not a blocker: #6123. I'll tackle this in a new PR.

Also, I pushed some minor changes:

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6150/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Thanks for moving this forward, @miukimiu!

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

Successfully merging this pull request may close these issues.

3 participants