-
Notifications
You must be signed in to change notification settings - Fork 844
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 EuiFormLayoutControl icons (3/3) #7959
Conversation
+ remove modifier classes - use ternaries to avoid CSS unsetting
- bonus: this allows left icons such as EuiFieldPassword and EuiSearch to also adjust their icon color
…ormControlLayout` - 🤦 *now* EuiFieldPassword and EuiFieldSearch should correctly gray out their icons when disabled
- `size` CSS isn't doiing anything, size is set via prop depending on compressed prop - `.euiFormControlLayoutCustomIcon__icon` isn't doing anything, the flex CSS on `EuiFormControlLayoutIcons` already handles centering - `:focus` CSS is entirely unnecessary, that's already handled by browsers and is displaying default browser focus rings
…ed when the parent input is disabled + fix `disabled` prop being incorrectly passed to `<span />` tags
- static and minimal, make it an inline obj
TSX: - remove className map - remove wrapping EuiI18n in favor of hook CSS: - remove unnecessary specific sizes in favor of percentages (50%, 0.5) - remove unnecessary `:focus` CSS (same as default browser focus ring)
a8ca11b
to
ae191ed
Compare
- add control to storybook for easier QA - update clear button to more obviously note disabled status (generally should just be removed if the input is disabled, so this is just in case)
ae191ed
to
7a1e89e
Compare
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.
🚢 🐈⬛ Nice cleanup - The changes looks great! I didn't notice any visual regression 👏
First time for everything! 😄 Thanks a million Lene!! |
<EuiIcon | ||
css={iconStyles} | ||
className="euiFormControlLayoutClearButton__icon" | ||
type="cross" |
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.
Oh, I forgot to leave a comment about this! I experimented with replacing this with the new crossInCircle
icon that was recently added, but it didn't look good at small sizes unfortunately, so I left the current cross
icon as-is with custom CSS.
💚 Build Succeeded
History
|
`v95.7.0` ⏩ `v95.9.0` > [!CAUTION] > This PR contains the final set of Emotion conversions for all EuiForm components. > If your plugin contains any custom CSS/styling to **EuiFormRow, EuiFormControlLayout, EuiCheckbox, EuiRadio, or EuiSwitch**,⚠️ make sure to QA your UI to ensure no visual regressions have occurred!⚠️ --- ## [`v95.9.0`](https://github.com/elastic/eui/releases/v95.9.0) - Updated `EuiSearchBar`'s optional `box.schema` prop with a new `recognizedFields` configuration. This allows specifying the phrases that will be parsed as field clauses ([#7960](elastic/eui#7960)) - Updated `EuiIcon` with a new `tokenSemanticText` glyph ([#7971](elastic/eui#7971)) - Added support for TypeScript 5 ([#7980](elastic/eui#7980)) **Bug fixes** - Fixed `EuiSelectableTemplateSitewide` styles when used within a dark-themed `EuiHeader` ([#7977](elastic/eui#7977)) ## [`v95.8.0`](https://github.com/elastic/eui/releases/v95.8.0) - Updated `EuiHeaderLinks`'s mobile menu to set a slight popover padding by default ([#7961](elastic/eui#7961)) - This can be overridden via `popoverProps.panelPaddingSize` if needed. - Updated `EuiHeaderLink` to default to a size of `s` (down from `m`) ([#7961](elastic/eui#7961)) **Accessibility** - Updated the `aria-label` attribute for the `EuiFieldSearch` clear button ([#7970](elastic/eui#7970)) **Bug fixes** - Fixed a visual bug with `<EuiDualRange showInput="inputWithPopover" />` form controls ([#7957](elastic/eui#7957)) **Deprecations** - Deprecated `EuiFormRow`'s `columnCompressedSwitch` display prop. Use `columnCompressed` instead, which will automatically account for child `EuiSwitch`es ([#7968](elastic/eui#7968)) - Deprecated `EuiFormRow`'s `rowCompressed` display prop. Use `row` instead for vertical forms, or `centerCompressed` for inline forms ([#7968](elastic/eui#7968)) - (Styling) Updated `EuiFormRow`'s `hasEmptySpaceLabel` prop to no longer attempt to automatically align its content to a vertical center. Use the `display="center"` prop for that instead ([#7968](elastic/eui#7968)) **CSS-in-JS conversions** - Converted `EuiFormControlLayout` to Emotion ([#7954](elastic/eui#7954)) - Removed `.euiFormControlLayout--*icons` classNames and `--eui-form-control-layout-icons-padding` CSS var. Use `--euiFormControlRightIconsCount` or `--euiFormControlLeftIconsCount` instead - Converted `EuiFormLayoutDelimited` to Emotion ([#7957](elastic/eui#7957)) - Fixed `cloneElementWithCss` throwing an error when used multiple times without a `key` prop ([#7957](elastic/eui#7957)) - Updated `cloneElementWithCss` utility to support a third argument that allows prepending vs. appending the cloned Emotion css className ([#7957](elastic/eui#7957)) - Removed `@euiFormControlLayoutClearIcon` Sass mixin ([#7959](elastic/eui#7959)) - Converted `EuiDescribedFormGroup` to Emotion ([#7964](elastic/eui#7964)) - Converted `EuiForm`, `EuiFormHelpText`, and `EuiFormErrorText` to Emotion ([#7966](elastic/eui#7966)) - Converted `EuiFormLabel` and `EuiFormLegend` to Emotion; Removed `@euiFormLabel` mixin ([#7967](elastic/eui#7967)) - Converted `EuiFormRow` to Emotion ([#7968](elastic/eui#7968)) - Converted `EuiCheckbox` to Emotion ([#7969](elastic/eui#7969)) - Converted `EuiRadio` to Emotion ([#7969](elastic/eui#7969)) - Converted `EuiSwitch` to Emotion ([#7969](elastic/eui#7969)) - Removed the following Sass variables: ([#7969](elastic/eui#7969)) - `$euiFormCustomControlDisabledIconColor` - `$euiFormCustomControlBorderColor` - `$euiRadioSize` - `$euiCheckBoxSize` - `$euiCheckboxBorderRadius` - `$euiSwitchHeight` (and compressed/mini variants) - `$euiSwitchWidth` (and compressed/mini variants) - `$euiSwitchThumbSize` (and compressed/mini variants) - `$euiSwitchIconHeight` - `$euiSwitchOffColor` - Removed the following Sass mixins: ([#7969](elastic/eui#7969)) - `euiIconBackground` - `euiCustomControl` - `euiCustomControlSelected` - `euiCustomControlDisabled` - `euiCustomControlFocused` --------- Co-authored-by: Marta Bondyra <[email protected]>
Summary
Follow up to #7954
As always, please review by commit - there's plenty of cleanup, some fixes around disabled styling, and a bunch of downstream snapshot changes.
QA
The below links/steps should look the same as production:
icon.side
toleft
and confirm that the :) face moves correctlyonClick: true
key/value to theicon
object and confirm that icon becomes a focusable buttonisDisabled
toggle and confirm that the :) face and clear button both correctly fade out and show a cursor indicating disabled statusloading
andinvalid
statescompressed
state and confirm the clear button looks the same as production when smallGeneral checklist
- [ ] Added or updated jest and 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
shouldRenderCustomStyles()
test was added and passes with parent component and any nestedchildProps
(e.g.tooltipProps
)[ ] Converted Enzyme to 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
gap
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
)Kibana 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)~ None, thankfullyExtras/nice-to-have
[ ] 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 componentRemoved modifier classes:
.euiFormControlLayoutIcons--absolute
,.euiFormControlLayoutIcons--static
,.euiFormControlLayoutIcons--left
,.euiFormControlLayoutIcons--right
.euiFormControlLayoutCustomIcon--clickable
.euiFormControlLayoutClearButton--small
Biggest direct usage TODO to update: