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 EuiFilePicker #7833

Merged
merged 17 commits into from
Jun 18, 2024
Merged

Conversation

cee-chen
Copy link
Contributor

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

Summary

This PR converts EuiFilePicker to Emotion. It also contains a few opinionated style cleanups:

  1. default (non-large) file picker display has had its font colors tweaked to match large displays (change originally made in EuiFilePicker dropzone style updates #6479)
Before After
  1. compressed+default file pickers now resize their icons to match other compressed form controls
Before After
Comparison against other compressed form controls
  1. The slight background color on compressed file pickers has been removed (to be honest, I'm not totally sure why it was ever there in the first place)

  2. Removed unnecessary .euiFilePicker__wrap DOM element

QA

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
  • Release checklist
    • A changelog entry exists and is marked appropriately.
      - [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist
    • Updated the Figma library counterpart

Figma does need updating, it was never updated during #6479

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 No playground

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) - did remove, but checked first - no Kibana usages
  • 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)

2 custom CSS styles that will need to be evaluated:


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

I spiked out converting the component to a function component but decided against it due to the removeFiles ref usage in Kibana.

Sorry, something went wrong.

cee-chen added 15 commits June 12, 2024 16:46
+ remove necessary CSS
- simplify/remove mixin by using component directly instead
- not a super huge fan of how nest-y this is, but not sure there's a better way of writing this
- Move majority of CSS to `euiFilePicker__prompt` wrapper - the text styling doesn't need to be overly specific, except for the hover/focus underline

- change element from div to span to indicate that it's not meant to be a layout element

- simplify line-height and margin offset

- [opinionated change] Tweak normal control color and font weight to match non-large control
@cee-chen cee-chen marked this pull request as ready for review June 13, 2024 17:22
@cee-chen cee-chen requested a review from a team as a code owner June 13, 2024 17:22
@mgadewoll
Copy link
Contributor

@cee-chen Should this PR merge into main? Or should the next batch of form control Emotion conversions be batched together again in a separate feature branch?

@cee-chen
Copy link
Contributor Author

Yes, it should merge into main. EuiFilePicker is kind of a weird unicorn and doesn't look like any other form component, so it's pretty safe to merge standalone

@@ -52,7 +51,7 @@ export interface EuiFilePickerProps
* `default` for normal height, similar to other controls;
* `large` for taller size
*/
display?: EuiFilePickerDisplay;
display?: 'default' | 'large';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to add default value markers. We lost that info in both Storybook and the EUI docs now with the conversion for these props: compressed, display and initialPromptText.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh thanks for catching this!

Copy link
Contributor Author

@cee-chen cee-chen Jun 18, 2024

Choose a reason for hiding this comment

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

I wonder what caused defaultProps to stop getting parsed automatically though. I think it was the theme HOC, which is a little frustrating. I debated converting this component to a function component, but the fact that we expose class methods via ref made me pause that work 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@mgadewoll mgadewoll Jun 18, 2024

Choose a reason for hiding this comment

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

It might be the added level of abstraction, yes. Also just as is. So far to me it seems that the props table generator that both Storybook and EUI use have trouble resolving complex types as well as too "abstracted away" types. I haven't really understood the "pattern" yet though either. That would required its own "little" investigation I think.

…e now missing
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 had checked the changes manually and did not notice any issues apart from the mentioned expected changes. Thanks for the additional default prop update! Looks good to me now 👍

@cee-chen
Copy link
Contributor Author

Thanks a million Lene!!

@cee-chen cee-chen enabled auto-merge (squash) June 18, 2024 17:37
@kibanamachine
Copy link

Preview staging links for this PR:

@cee-chen cee-chen merged commit dc5b027 into elastic:main Jun 18, 2024
5 checks passed
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen cee-chen deleted the emotion/file-picker branch June 18, 2024 17:43
jbudz added a commit to elastic/kibana that referenced this pull request Jun 28, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
`v95.1.0`⏩`v95.2.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

## [`v95.2.0`](https://github.com/elastic/eui/releases/v95.2.0)

- Updated `EuiContextMenuItemIcon`'s type definition to explicitly
define support for `EuiIcon`'s `IconType`
([#7804](elastic/eui#7804))
- Updated `EuiSteps` to support a new `titleSize="xxs"` style, which
outputs the same title font size but smaller unnumbered step indicators
([#7813](elastic/eui#7813))
- Updated `EuiStepsHorizontal` to support a new `size="xs"` style, which
outputs smaller unnumbered step indicators
([#7813](elastic/eui#7813))
- Updated `EuiStepNumber` to support new `titleSize="none"` which omits
rendering step numbers, and will only render icons
([#7813](elastic/eui#7813))
- Updated `setEuiDevProviderWarning` to additionally accept a custom
callback function, which warning messages will be passed to
([#7820](elastic/eui#7820))
- Updated `EuiIcon` to feature updated `logoElasticStack` logo for
referencing Elastic Stack platform
([#7838](elastic/eui#7838))
- Updated `EuiIcon` to feature updated `casesApp` design.
([#7840](elastic/eui#7840))
- Updated `EuiComboBox` to no longer autocomplete searched text when
used within forms ([#7842](elastic/eui#7842))

**CSS-in-JS conversions**

- Converted `EuiFilePicker` to Emotion; Removed
`$euiFilePickerTallHeight`
([#7833](elastic/eui#7833))

---------

Co-authored-by: Jon <jon@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants