-
Notifications
You must be signed in to change notification settings - Fork 842
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 EuiSearchBar #7490
Conversation
import { euiFormVariables } from '../form/form.styles'; | ||
|
||
export const euiSearchBar__searchHolder = (euiThemeContext: UseEuiTheme) => { | ||
const { maxWidth } = euiFormVariables(euiThemeContext); |
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.
This very likely has performance implications as well (see #7486) and we likely need to memoize euiFormVariables
(and other CSS-in-JS component style vars) going forward. I'll address that in a separate PR from this one
Preview staging links for this PR:
|
💚 Build Succeeded
|
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.
LGTM. I had one question about logical CSS properties that's not a blocker.
} | ||
/> | ||
</EuiFlexItem> | ||
{filters && ( |
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.
DELETED
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.
While I was here, I also refactored some of the rendered JSX to not use a variable unnecessarily (which as react perf implications) but instead as an inline JSX conditional.
That's wild that this has a performance implication, I had no idea. I assume this is what you're referring to. I'm going to link to this change specifically as a reference on #7499
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.
Kevin chatted about me with this in relation to #7374, but iirc the issue is that React creates the conditional node let
as a new React component every single rerender. @kqualters-elastic feel free to chime in if I've misquoted you!
`v92.2.1` ⏩ `v93.0.0` --- ## [`v93.0.0`](https://github.com/elastic/eui/releases/v93.0.0) **Bug fixes** - Fixed `EuiTextTruncate` component to clean up timer from side effect on unmount ([#7495](elastic/eui#7495)) **Breaking changes** - Removed deprecated `anchorClassName` prop from `EuiPopover`. Use `className` instead ([#7488](elastic/eui#7488)) - Removed deprecated `buttonRef` prop from `EuiPopover`. Use `popoverRef` instead ([#7488](elastic/eui#7488)) - Removed deprecated `toolTipTitle` and `toolTipPosition` props from `EuiContextMenuItem`. Use `toolTipProps.title` and `toolTipProps.position` instead ([#7489](elastic/eui#7489)) - Removed deprecated internal `setSelection` ref method from `EuiInMemoryTable` and `EuiBasicTable`. Use the new controlled `selection.selected` prop API instead. ([#7491](elastic/eui#7491)) - `EuiTourStep`'s `className` and `style` props now apply to the anchoring element instead of to the popover panel, to match `EuiPopover` behavior. ([#7497](elastic/eui#7497)) - Convert your existing usages to `panelClassName` and `panelStyle` respectively instead. **Performance** - Improved the amount of recomputed styles being generated by `EuiCode` and `EuiCodeBlock` ([#7486](elastic/eui#7486)) **CSS-in-JS conversions** - Converted `EuiSearchBar` to Emotion ([#7490](elastic/eui#7490)) - Converted `EuiEmptyPrompt` to Emotion ([#7494](elastic/eui#7494)) - Added `euiBorderColor` and `useEuiBorderColorCSS` style utilities ([#7494](elastic/eui#7494)) --------- Co-authored-by: Jon <jon@elastic.co>
`v92.2.1` ⏩ `v93.0.0` --- ## [`v93.0.0`](https://github.com/elastic/eui/releases/v93.0.0) **Bug fixes** - Fixed `EuiTextTruncate` component to clean up timer from side effect on unmount ([elastic#7495](elastic/eui#7495)) **Breaking changes** - Removed deprecated `anchorClassName` prop from `EuiPopover`. Use `className` instead ([elastic#7488](elastic/eui#7488)) - Removed deprecated `buttonRef` prop from `EuiPopover`. Use `popoverRef` instead ([elastic#7488](elastic/eui#7488)) - Removed deprecated `toolTipTitle` and `toolTipPosition` props from `EuiContextMenuItem`. Use `toolTipProps.title` and `toolTipProps.position` instead ([elastic#7489](elastic/eui#7489)) - Removed deprecated internal `setSelection` ref method from `EuiInMemoryTable` and `EuiBasicTable`. Use the new controlled `selection.selected` prop API instead. ([elastic#7491](elastic/eui#7491)) - `EuiTourStep`'s `className` and `style` props now apply to the anchoring element instead of to the popover panel, to match `EuiPopover` behavior. ([elastic#7497](elastic/eui#7497)) - Convert your existing usages to `panelClassName` and `panelStyle` respectively instead. **Performance** - Improved the amount of recomputed styles being generated by `EuiCode` and `EuiCodeBlock` ([elastic#7486](elastic/eui#7486)) **CSS-in-JS conversions** - Converted `EuiSearchBar` to Emotion ([elastic#7490](elastic/eui#7490)) - Converted `EuiEmptyPrompt` to Emotion ([elastic#7494](elastic/eui#7494)) - Added `euiBorderColor` and `useEuiBorderColorCSS` style utilities ([elastic#7494](elastic/eui#7494)) --------- Co-authored-by: Jon <jon@elastic.co>
`v92.2.1` ⏩ `v93.0.0` --- ## [`v93.0.0`](https://github.com/elastic/eui/releases/v93.0.0) **Bug fixes** - Fixed `EuiTextTruncate` component to clean up timer from side effect on unmount ([elastic#7495](elastic/eui#7495)) **Breaking changes** - Removed deprecated `anchorClassName` prop from `EuiPopover`. Use `className` instead ([elastic#7488](elastic/eui#7488)) - Removed deprecated `buttonRef` prop from `EuiPopover`. Use `popoverRef` instead ([elastic#7488](elastic/eui#7488)) - Removed deprecated `toolTipTitle` and `toolTipPosition` props from `EuiContextMenuItem`. Use `toolTipProps.title` and `toolTipProps.position` instead ([elastic#7489](elastic/eui#7489)) - Removed deprecated internal `setSelection` ref method from `EuiInMemoryTable` and `EuiBasicTable`. Use the new controlled `selection.selected` prop API instead. ([elastic#7491](elastic/eui#7491)) - `EuiTourStep`'s `className` and `style` props now apply to the anchoring element instead of to the popover panel, to match `EuiPopover` behavior. ([elastic#7497](elastic/eui#7497)) - Convert your existing usages to `panelClassName` and `panelStyle` respectively instead. **Performance** - Improved the amount of recomputed styles being generated by `EuiCode` and `EuiCodeBlock` ([elastic#7486](elastic/eui#7486)) **CSS-in-JS conversions** - Converted `EuiSearchBar` to Emotion ([elastic#7490](elastic/eui#7490)) - Converted `EuiEmptyPrompt` to Emotion ([elastic#7494](elastic/eui#7494)) - Added `euiBorderColor` and `useEuiBorderColorCSS` style utilities ([elastic#7494](elastic/eui#7494)) --------- Co-authored-by: Jon <jon@elastic.co>
Summary
This is a pretty quick/mildly dirty Emotion conversion of
EuiSearchBar
, due to the nature of the class component (harder to use hooks for) and very minor CSS (only 2 rules, nothing on the main wrapper, just child elements). I strongly recommend viewing the diff with whitespace changes hidden.While I was here, I also refactored some of the rendered JSX to not use a variable unnecessarily (which has react perf implications) but instead as an inline JSX conditional.
QA
General checklist
Emotion checklist
Kibana usage
**/target, **/*.snap, **/*.storyshot
for less noise) foreui{Component}
(case sensitive) to find:[ ] Search Kibana's codebase for- No modifier classes changed{euiComponent}-
(case sensitive) to check for usage of modifier classeseuiBadge
class on a div instead of simply using theEuiBadge
component) - NoneGeneral
className(s)
read as expected in snapshots and browsers[ ] Checked component playgroundNo playgroundUnit tests
[ ]shouldRenderCustomStyles()
test was added and passes with parent component and any nestedchildProps
(e.g.tooltipProps
)Sass/Emotion conversion process
$euiSize
toeuiTheme.size.base
)[ ] Removed or converted component-specific Sass vars/mixins to exported JS versions[ ] Listed var/mixin removals in changelog[ ] Simplifiedcalc()
tomathWithUnits
if possible (if mixing different unit types, this may not be possible)[ ] Added an@warn
deprecation message within theglobal_styling/mixins/{component}.scss
file[ ] All SCSS overrides have been removed from the Amsterdam themeNo overridesCSS 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 DOM or className changes/removalsExtras/nice-to-have
[ ] Reduced specificity where possible (usually by reducing nesting and class name chaining)Wasn't necessary[ ] Documentation pass[ ] Check for issues in the backlog that could be a quick fix for that component[ ] Optional component/code cleanup: consider splitting up the component into multiple children if it's overly verbose or difficult to reason about