eui icon indicating copy to clipboard operation
eui copied to clipboard

[Emotion] Add form variables and mixins

Open elizabetdev opened this issue 3 years ago • 2 comments

Summary

The idea of this PR is to help with the conversion of the form's components to Emotion. So let's say this is step one. Then I can start grabbing some form components to convert.

So basically this PR adds src/components/form/form.styles.ts with variables and a few mixins. I started this conversion in #6116 but it seems that the PR is going to take time and I don't want to waste time.

  • All the variables that start with $euiForm from src/global_styling/variables/_form.scss were converted to js.
  • The variables that star with $euiSwitch, $euiRadio, $euiRange, were not converted. Likely those variables are going to live in the components folder.
  • A few mixins contained in src/global_styling/mixins/_form.scss were also converted to js. Not all. I just took the ones that I already converted in #6116.

It also consolidates all the Amsterdam and deprecated theme form mixins and variables. This means the following styles were deleted:

  • src/themes/amsterdam/global_styling/mixins/_form.scss
  • src/themes/amsterdam/global_styling/variables/_index.scss
  • src/themes/amsterdam/global_styling/variables/_borders.scss

And the all styles contained on the above files were moved to:

  • src/global_styling/mixins/_form.scss
  • src/global_styling/variables/_form.scss
  • src/global_styling/variables/_borders.scss

How to test

You can test the variables and mixins locally or you can see the variables and colors here: https://eui.elastic.co/pr_6256/#/forms/form-controls

Screenshot 2022-09-21 at 16 35 29 Screenshot 2022-09-21 at 16 35 36

Checklist

  • [ ] 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
  • [ ] Revert the [Revert Me] commit

elizabetdev avatar Sep 21 '22 15:09 elizabetdev

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

kibanamachine avatar Sep 21 '22 16:09 kibanamachine

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

kibanamachine avatar Sep 21 '22 18:09 kibanamachine

jenkins test this

elizabetdev avatar Sep 22 '22 10:09 elizabetdev

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

kibanamachine avatar Sep 22 '22 10:09 kibanamachine

That's it for my review, I mostly just had minor syntax nits around the new JS. LMK if you'd rather me push up my change requests directly to the branch.

The revert / docs commit made it super easy to QA, thank you for doing that!! 🙌

cee-chen avatar Sep 22 '22 17:09 cee-chen

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

kibanamachine avatar Sep 23 '22 12:09 kibanamachine

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

kibanamachine avatar Sep 23 '22 14:09 kibanamachine

👋 Changes look great, Elizabet! After thinking it over, since these vars and utils are relatively complex and will be reused by other components, I'd like to write unit tests for them + snapshot outputs. I'll go ahead and push them up to your branch if no objections!

cee-chen avatar Sep 23 '22 16:09 cee-chen

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

kibanamachine avatar Sep 23 '22 17:09 kibanamachine

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

kibanamachine avatar Sep 23 '22 18:09 kibanamachine

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

kibanamachine avatar Sep 26 '22 11:09 kibanamachine