Skip to content

Comments

[Emotion] Convert basic form controls#7823

Merged
cee-chen merged 11 commits intomainfrom
emotion/forms
Jun 11, 2024
Merged

[Emotion] Convert basic form controls#7823
cee-chen merged 11 commits intomainfrom
emotion/forms

Conversation

@cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jun 10, 2024

Summary

Converts the following basic form controls to Emotion:

  • EuiFieldText
  • EuiFieldNumber
  • EuiFieldSearch
  • EuiFieldPassword
  • EuiTextArea
  • EuiSelect
  • EuiSuperSelect

See #6400

QA

The majority of QA should have been performed in the above PRs, so this step is one final sanity check / smoke test:

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
      - [ ] Checked in Chrome, Safari, Edge, and Firefox
      - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA - N/A
  • Code quality checklist - N/A, feature branch
  • Release checklist - N/A, feature branch - individual PRs have their own changelogs
  • Designer checklist - N/A
Emotion checklist

General

  • Output CSS matches the previous CSS (works as expected in all browsers)
  • Rendered className(s) read as expected in snapshots and browsers
  • Checked component playground

    NOTE: Class components wrapped in withEuiTheme need to pass true as the second argument to its propUtilityForPlayground in src-docs/src/views/{component}/playground.js


Unit tests

  • shouldRenderCustomStyles() test was added and passes with parent component and any nested childProps (e.g. tooltipProps)
  • Removed any mount()ed snapshots in favor of render() or a more specific assertion

Sass/Emotion conversion process

  • [ ] Converted all global Sass vars/mixins to JS (e.g. $euiSize to euiTheme.size.base) - N/A, this will be done last at the end of the full forms conversion
  • [ ] Listed var/mixin removals in changelog
  • [ ] Added an @warn deprecation message within the global_styling/mixins/{component}.scss file
  • Removed or converted component-specific Sass vars/mixins to exported JS versions
  • [ ] Ran yarn compile-scss to update var/mixin JSON files
  • [ ] Simplified calc() to mathWithUnits if possible (if mixing different unit types, this may not be possible)
  • Removed component from src/components/index.scss
  • [ ] Deleted any src/amsterdam/overrides/{component}.scss files (styles within should have been converted to the baseline Emotion styles)

CSS tech debt

  • Wrapped all animations or transitions in euiCanAnimate
  • Used gap property to add margin between items if using flex
  • Converted side specific padding, margin, and position to -inline and -block logical properties (check inline styles as well as CSS)

DOM Cleanup

  • Did NOT remove any block/element classNames (e.g. euiComponent, euiComponent__child)
  • SEARCH KIBANA FIRST: Deleted any modifier classNames or maps if not being used in Kibana.

Kibana due diligence

  • Search Kibana's codebase for {euiComponent}- (case sensitive) to check for usage of modifier classes
    • [ ] If usage exists, consider converting to a data attribute so that consumers still have something to hook into
  • Pre-emptively check how your conversion will impact the next Kibana upgrade. This entails searching/grepping through Kibana (excluding **/target, **/*.snap, **/*.storyshot for less noise) for eui{Component} (case sensitive) to find:
  • Any test/query selectors that will need to be updated
  • Any Sass or CSS that will need to be updated, particularly if a component Sass var was deleted
  • Any direct className usages that will need to be refactored (e.g. someone calling the euiBadge class on a div instead of simply using the EuiBadge component)

Extras/nice-to-have

  • Reduced specificity where possible (usually by reducing nesting and class name chaining)
  • [ ] Optional component/code cleanup: consider splitting up the component into multiple children if it's overly verbose or difficult to reason about
  • [ ] Documentation pass
  • [ ] Check for issues in the backlog that could be a quick fix for that component

@cee-chen cee-chen added skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) emotion labels Jun 10, 2024
@cee-chen cee-chen marked this pull request as ready for review June 10, 2024 18:42
@cee-chen cee-chen requested a review from a team as a code owner June 10, 2024 18:42
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 🐈‍⬛ I ran another manual check over the docs for the form components and all looks good to me 👍

🗒️ There are expected changes for:

  • spacing between icons and input values/placeholders
  • option size for EuiSuperSelect

@cee-chen
Copy link
Contributor Author

Thanks a million Lene!!

@cee-chen cee-chen merged commit 5e96b57 into main Jun 11, 2024
@cee-chen cee-chen deleted the emotion/forms branch June 11, 2024 16:56
@cee-chen cee-chen restored the emotion/forms branch June 11, 2024 17:12
@cee-chen cee-chen deleted the emotion/forms branch June 24, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

emotion skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants