-
Notifications
You must be signed in to change notification settings - Fork 829
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 EuiSelectableTemplateSitewide #7944
[Emotion] Convert EuiSelectableTemplateSitewide #7944
Conversation
- this requires passing the styles obj to the `euiSelectableTemplateSitewideFormatOptions` function, which isn't actually a react component :| + remove unnecessary `!important` + fix import order/unnecessary extra dir
- Ignore `$euiSelectableTemplateFocusBackgroundLight/Dark` - we can't meaningfully use this background in our chroma.js calculations as chroma can't calculate contrast w/ alpha transparency. Using the default body background color should suffice / generates the same output colors
- again, these files aren't React components so we have to continuously pass the memoized style obj around, and use `RenderWithEuiStylesMemoizer` :T - [syntax] convert `renderOptionMeta` to obj arg notation for easier DX and fallbacks
- render functions don't snapshot well, so we should just render the DOM + update the `searchValue` test to be marginally more useful/pointed
- not a lot we can for `__listItem`, but we can at least dogfood `euiSelectableTemplateSitewideRenderOptions` children
- doesn't appear to be needed anymore, not totally sure why tho
+ add a vrt-only one to allow for snapshotting options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking great! I noticed one small regression on content position, not sure if it's intended?
Screen.Recording.2024-08-06.at.17.22.18.mov
...s/eui/src/components/selectable/selectable_templates/selectable_template_sitewide_option.tsx
Show resolved
Hide resolved
I think it's intended and it's due to the line-height changes/reductions Caroline implemented as part of the Emotion conversion. Also, it looks better and more balanced this way (just my 2 cents!) |
Yeah I'm fine with it too, I just didn't understand where it came from, so I figured better confirm. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢 🐈⬛ Thanks for the additional explanation; looking good to me! 🚀
💚 Build Succeeded
History
|
`v95.6.0` ⏩ `v95.7.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.7.0`](https://github.com/elastic/eui/releases/v95.7.0) **CSS-in-JS conversions** - Converted `EuiSelectable` to Emotion ([#7940](elastic/eui#7940)) - Removed `$euiSelectableListItemBorder` - Removed `$euiSelectableListItemPadding` - Converted `EuiSelectableTemplateSitewide` to Emotion ([#7944](elastic/eui#7944)) - Removed `$euiSelectableTemplateFocusBackgroundLight` - Removed `$euiSelectableTemplateFocusBackgroundDark` - Removed `$euiSelectableTemplateSitewideTypes` - Converted `EuiComboBox` to Emotion ([#7950](elastic/eui#7950))
Summary
Follow up to #7940
EuiSelectableTemplateSitewide has some weirdness where the options file doesn't actually contain any React components, so some extra shenanigans with passing around styles is needed. As always, I recommend following along by commit.
QA
General checklist
and screenreader modesclassName
s directlyand cypress tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)Emotion checklist
General
className(s)
read as expected in snapshots and browsers[ ] Checked component playgroundUnit tests
[ ]- already done previouslyshouldRenderCustomStyles()
test was added and passes with parent component and any nestedchildProps
(e.g.tooltipProps
)[ ] Converted Enzyme to RTL- already RTLSass/Emotion conversion process
src/components/index.scss
$euiSize
toeuiTheme.size.base
)[ ] Added an@warn
deprecation message within theglobal_styling/mixins/{component}.scss
file[ ] Removed or converted component-specific Sass vars/mixins to exported JS versions[ ] Ranyarn compile-scss
to update var/mixin JSON files[ ] Simplifiedcalc()
tomathWithUnits
if possible (if mixing different unit types, this may not be possible)[ ] Deleted anysrc/amsterdam/overrides/{component}.scss
files (styles within should have been converted to the baseline Emotion styles)CSS tech debt
[ ] Wrapped all animations or transitions ineuiCanAnimate
[ ] Usedgap
property to add margin between items if using flex-inline
and-block
logical properties (check inline styles as well as CSS)DOM Cleanup
euiComponent
,euiComponent__child
)[ ] SEARCH KIBANA FIRST: Deleted any modifier classNames or maps if not being used in Kibana.- no modifiers removedKibana due diligence
{euiComponent}-
(case sensitive) to check for usage of modifier classes[ ] If usage exists, consider converting to adata
attribute so that consumers still have something to hook into**/target, **/*.snap, **/*.storyshot
for less noise) foreui{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 deletedeuiBadge
class on a div instead of simply using theEuiBadge
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 component2 Kibana direct className usages that need to be checked: