Skip to content

Commit

Permalink
[Emotion] Fix consumer css overriding (instead of merging with) EUI c…
Browse files Browse the repository at this point in the history
…ss in child props (#6896)

* [setup] Document `shouldRenderCustomStyles` options type more cleanly

- since I'll be adding a few more needed options here shortly

* [setup] Move `assertOutputStyles` to internal util

- will be adding a few more here shortly, as well as extending the fn, and it's nice not to have to continously pass `options` around

* [setup] DRY out render + renderCallback to a util

- will be adding another util here shortly that will be using it, and 3 times is enough to DRY it out

* [setup] DRY out `childProps` merge util

- the next util we're adding will use it

* [FIX] Ensure that `css` props do not override baseline EUI `css`

- this is the main fix/guard/detection for the buggy behavior found in this PR

- it acts by rendering the baseline component w/ no custom `css`, grabbing the Emotion className generated, and ensuring the custom `css` is appended to the end of it instead of gobbling it

- this behavior primarily affects nested `childProps` components, as Emotion does not auto-merge `css` props that isn't the immediately returned element / isn't at the top level of the component

* [FIX] Fix `shouldRenderCustomStyles` not merging css no matter what

- Emotion handles merging automatically, so this essentially mimics end behavior but in test

* [EuiAccordion] Fix `buttonProps.css` and `arrowProps.css` overriding EUI css

* [EuiBadge] Fix `closeButtonProps.css` overriding EUI css

* [EuiCard] Fix `betaBadgeProps.css` and `betaBadgeProps.anchorProps.css` overriding EUI css

* [EuiExpression] Fix `descriptionProps` and `valueProps` overrides

- fix childProps `className` not being correctly merged
- fix `css` not being correctly merged
- rewrite `style` merge/overrides a bit more succinctly

* [EuiFlyout] Fix `closeButtonProps.css` overriding EUI css

* [EuiKeyPadMenu]  Fix `checkable.legendProps.css` overriding EUI css

+ DRY out checkable typeof logic

* [EuiListGroupItem]  Fix `iconProps.css` overriding EUI css

* [EuiPageBody] Fix `panelProps.css` overriding EUI css

- extracting `css` out and setting it to the end of the array ensures it will always correctly come after EUI styles

+ misc cleanup / better organization of code

+ add extra non-panelled test

* [EuiPageHeaderContent] Fix `iconProps.css` overriding EUI css

* [EuiProgress] Fix `labelProps.css` overriding EUI css

* [EuiTour] Fix `panelProps` test

- behavior is working, but because EuiTour has conflates some props with `panelProps`, we need to isolate and not pass any extra `css` (in `requiredProps`) other than what is automatically tested

* [enhancement] Add `options.wrapper` API to `shouldRenderCustomStyles`

+ update `EuiResizablePanel` to use this new `wrapper` options, which both significantly DRYs out behavior as well as allowing `shouldRenderCustomStyles` to clone props correctly

* [EuiResizablePanel] Fix `wrapperProps.css` overriding EUI css

* [EuiIcon] Fix failing svg test

- bogart the new wrapper API to preload `appendIconComponentCache`, otherwise the EUI class comparisons encounter race conditions with the `isLoading` CSS

* [EuiImage] Fix `wrapperProps.css` overriding EUI css

* [EuiImage] Fix fullscreen wrapper css + add `targetSelector` API to `shouldRenderCustomStyles`

- EuiImage renders multiple copies of an image wrapper on the page, so we need a specific way of targeting which DOM node to get the right Emotion classes from

+ convert remaining tests to RTL as cleanup/tech debt

+ remove skipped test entirely

* [opinionated] Remove custom `css_before_spread_props` lint rule

- As noted, Emotion does not auto-merge non-top-level props so we need to do it manually for css merging to work correctly.

This means a lot of disables - we're probably better off linting for the presence of `shouldRenderCustomStyles` instead, which actually tests for working Emotion CSS

* [docs] Add wiki documentation of this behavior

* changelog
  • Loading branch information
cee-chen authored Jul 5, 2023
1 parent 12731eb commit 6573580
Show file tree
Hide file tree
Showing 34 changed files with 365 additions and 525 deletions.
1 change: 0 additions & 1 deletion .eslintplugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,5 @@ exports.rules = {
'require-license-header': require('./scripts/eslint-plugin/require_license_header'),
'forward-ref': require('./scripts/eslint-plugin/forward_ref_display_name'),
'css-logical-properties': require('./scripts/eslint-plugin/css_logical_properties'),
css_before_spread_props: require('./scripts/eslint-plugin/css_before_spread_props'),
'require-cypress-references': require('./scripts/eslint-plugin/require_cypress_references'),
};
1 change: 0 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ module.exports = {
'local/href-with-rel': 'error',
'local/forward-ref': 'error',
'local/css-logical-properties': 'error',
'local/css_before_spread_props': 'error',
'local/require-cypress-references': 'error',
'local/require-license-header': [
'warn',
Expand Down
48 changes: 0 additions & 48 deletions scripts/eslint-plugin/css_before_spread_props.js

This file was deleted.

248 changes: 0 additions & 248 deletions scripts/eslint-plugin/css_before_spread_props.test.js

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ exports[`EuiAccordion props arrowProps is rendered 1`] = `
aria-expanded="false"
aria-label="aria-label"
aria-labelledby="generated-id"
class="euiButtonIcon euiAccordion__iconButton testClass1 testClass2 emotion-euiButtonIcon-xs-empty-text-euiTestCss"
class="euiButtonIcon euiAccordion__iconButton testClass1 testClass2 emotion-euiButtonIcon-xs-empty-text-euiAccordion__iconButton-euiTestCss"
data-test-subj="test subject string"
tabindex="-1"
type="button"
Expand Down Expand Up @@ -637,7 +637,7 @@ exports[`EuiAccordion props buttonProps is rendered 1`] = `
aria-controls="4"
aria-expanded="false"
aria-label="aria-label"
class="euiAccordion__button testClass1 testClass2 emotion-euiTestCss"
class="euiAccordion__button testClass1 testClass2 emotion-euiAccordion__button-euiTestCss"
data-test-subj="test subject string"
id="generated-id"
type="button"
Expand Down
6 changes: 4 additions & 2 deletions src/components/accordion/accordion.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ export class EuiAccordionClass extends Component<
const cssButtonStyles = [
buttonStyles.euiAccordion__button,
isDisabled && buttonStyles.disabled,
buttonProps?.css,
];

const childrenStyles = euiAccordionChildrenStyles(theme);
Expand All @@ -296,6 +297,7 @@ export class EuiAccordionClass extends Component<
iconButtonStyles.euiAccordion__iconButton,
isOpen && iconButtonStyles.isOpen,
_arrowDisplay === 'right' && iconButtonStyles.arrowRight,
arrowProps?.css,
];

const optionalActionStyles = euiAccordionOptionalActionStyles();
Expand All @@ -317,9 +319,9 @@ export class EuiAccordionClass extends Component<
iconButton = (
<EuiButtonIcon
color="text"
css={cssIconButtonStyles}
{...arrowProps}
className={iconButtonClasses}
css={cssIconButtonStyles}
iconType="arrowRight"
onClick={this.onToggle}
aria-controls={id}
Expand Down Expand Up @@ -369,10 +371,10 @@ export class EuiAccordionClass extends Component<

const button = (
<ButtonElement
css={cssButtonStyles}
{...buttonProps}
id={buttonId}
className={buttonClasses}
css={cssButtonStyles}
aria-controls={id}
// `aria-expanded` is only a valid attribute on interactive controls - axe-core throws a violation otherwise
aria-expanded={ButtonElement === 'button' ? isOpen : undefined}
Expand Down
4 changes: 2 additions & 2 deletions src/components/badge/badge.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ export const EuiBadge: FunctionComponent<EuiBadgeProps> = ({

const closeClassNames = classNames(
'euiBadge__icon',
closeButtonProps && closeButtonProps.className
closeButtonProps?.className
);

const Element = href && !isDisabled ? 'a' : 'button';
Expand Down Expand Up @@ -230,9 +230,9 @@ export const EuiBadge: FunctionComponent<EuiBadgeProps> = ({
type={iconType}
size="s"
color="inherit" // forces the icon to inherit its parent color
css={iconCssStyles}
{...closeButtonProps}
className={closeClassNames}
css={[...iconCssStyles, closeButtonProps?.css]}
/>
</button>
);
Expand Down
2 changes: 1 addition & 1 deletion src/components/card/card.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe('EuiCard', () => {

shouldRenderCustomStyles(
<EuiCard title="Card title" betaBadgeProps={{ label: 'beta' }} />,
{ childProps: ['betaBadgeProps'] }
{ childProps: ['betaBadgeProps', 'betaBadgeProps.anchorProps'] }
);

describe('props', () => {
Expand Down
Loading

0 comments on commit 6573580

Please sign in to comment.