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.

Sign up for GitHub

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

[EuiAccordion] Split up rendering into sub-components & other cleanups #7161

Merged
merged 12 commits into from
Sep 8, 2023
Merged

[EuiAccordion] Split up rendering into sub-components & other cleanups #7161

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a73bede
[REFACTOR] Split up accordion trigger into button & arrow sub-components
cee-chen Sep 2, 2023
5ca4269
[rename] Update downstream references to accordion arrow className
cee-chen Sep 4, 2023
255b5fb
[misc cleanup] EuiAccordion - reorder imports & props
cee-chen Sep 2, 2023
84d8479
[REFACTOR] Split up accordion children into sub-components
cee-chen Sep 2, 2023
f7773f4
[refactor] Switch to styles obj instead of modifying a DOM element di…
cee-chen Sep 4, 2023
87f4faa
[refactor] Remove manual tabbable controls and use `inert` attribute …
cee-chen Sep 4, 2023
8567a31
[opinionated refactor] Clean up child wrapper styles
cee-chen Sep 4, 2023
a72aa94
[regression] Fix children wrapper `.focus()` to only occur after mount
cee-chen Sep 4, 2023
3e82e7e
More downstream snapshot updates
cee-chen Sep 4, 2023
e195f62
changelog + minor stylelint cleanup
cee-chen Sep 6, 2023
dbcab02
[PR feedback] Reduce CSS overrides on arrow styles
cee-chen Sep 8, 2023
2d32730
:facepalm: Fix incorrect CSS
cee-chen Sep 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 48 additions & 48 deletions src/components/accordion/__snapshots__/accordion.test.tsx.snap
View file
Open in desktop

Large diffs are not rendered by default.

87 changes: 1 addition & 86 deletions src/components/accordion/accordion.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,7 @@

import { css } from '@emotion/react';
import { UseEuiTheme } from '../../services';
import {
euiFontSize,
logicals,
logicalCSS,
logicalTextAlignCSS,
} from '../../global_styling';
import { logicals, logicalCSS } from '../../global_styling';

export const euiAccordionStyles = ({ euiTheme }: UseEuiTheme) => {
return {
Expand All @@ -36,54 +31,6 @@ export const euiAccordionStyles = ({ euiTheme }: UseEuiTheme) => {
};
};

export const euiAccordionButtonStyles = (euiThemeContext: UseEuiTheme) => {
const { euiTheme } = euiThemeContext;
return {
euiAccordion__button: css`
${euiFontSize(euiThemeContext, 's')}
align-items: center;
display: flex;
flex-grow: 1;
line-height: ${euiTheme.size.l};
${logicalTextAlignCSS('left')}
${logicalCSS('width', '100%')}

&:hover,
&:focus {
cursor: pointer;
text-decoration: underline;
}
`,
// Triggering button needs separate `disabled` key because the element that renders may not support `:disabled`;
// Hover pseudo selector for specificity
disabled: css`
&,
&:hover {
cursor: not-allowed;
color: ${euiTheme.colors.disabledText};
text-decoration: none;
}
`,
// Optional padding sizes
s: css`
padding: ${euiTheme.size.s};
`,
m: css`
padding: ${euiTheme.size.base};
`,
l: css`
padding: ${euiTheme.size.l};
`,
// Remove padding from the accordion button on the side that the arrow is on
arrowLeft: css`
${logicalCSS('padding-left', 0)}
`,
arrowRight: css`
${logicalCSS('padding-left', 0)}
`,
};
};

export const euiAccordionChildrenStyles = ({ euiTheme }: UseEuiTheme) => ({
euiAccordion__children: css``,
isLoading: css`
Expand Down Expand Up @@ -128,40 +75,8 @@ export const euiAccordionChildWrapperStyles = ({ euiTheme }: UseEuiTheme) => ({
`,
});

export const euiAccordionIconButtonStyles = ({ euiTheme }: UseEuiTheme) => ({
euiAccordion__iconButton: css`
flex-shrink: 0;
${logicalCSS('margin-right', euiTheme.size.xs)}
transform: rotate(
0deg
) !important; /* stylelint-disable-line declaration-no-important */
`,
isOpen: css`
transform: rotate(
90deg
) !important; /* stylelint-disable-line declaration-no-important */
`,
arrowRight: css`
${logicalCSS('margin-left', euiTheme.size.xs)}
${logicalCSS('margin-right', 0)}
`,
});

export const euiAccordionOptionalActionStyles = () => ({
euiAccordion__optionalAction: css`
flex-shrink: 0;
`,
});

export const euiAccordionSpinnerStyles = ({ euiTheme }: UseEuiTheme) => ({
euiAccordion__spinner: css`
${logicalCSS('margin-right', euiTheme.size.xs)}
`,
});

export const euiAccordionTriggerWrapperStyles = () => ({
euiAccordion__triggerWrapper: css`
align-items: center;
display: flex;
`,
});
155 changes: 21 additions & 134 deletions src/components/accordion/accordion.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,13 @@ import {
withEuiTheme,
WithEuiThemeProps,
} from '../../services';
import { EuiButtonIcon, EuiButtonIconProps } from '../button';
import type { EuiButtonIconProps } from '../button';
import { EuiAccordionTrigger } from './accordion_trigger';
import {
euiAccordionStyles,
euiAccordionButtonStyles,
euiAccordionChildrenStyles,
euiAccordionChildWrapperStyles,
euiAccordionIconButtonStyles,
euiAccordionOptionalActionStyles,
euiAccordionSpinnerStyles,
euiAccordionTriggerWrapperStyles,
} from './accordion.styles';
import { logicalCSS } from '../../global_styling';

Expand Down Expand Up @@ -279,28 +276,12 @@ export class EuiAccordionClass extends Component<
isLoading,
isLoadingMessage,
isDisabled,
buttonProps: _buttonProps,
buttonElement: _ButtonElement = 'button',
buttonProps,
buttonElement,
arrowProps,
theme,
...rest
} = this.props;
const {
paddingSize: buttonPaddingSize,
className: buttonPropsClassName,
css: buttonPropsCss,
...buttonProps
} = _buttonProps || {};

// Force button element to be a legend if the element is a fieldset
const ButtonElement = Element === 'fieldset' ? 'legend' : _ButtonElement;
const buttonElementIsFocusable = ButtonElement === 'button';

// Force visibility of arrow button if button element is not focusable
const _arrowDisplay =
arrowDisplay === 'none' && !buttonElementIsFocusable
? 'left'
: arrowDisplay;

const classes = classNames(
'euiAccordion',
Expand All @@ -321,41 +302,6 @@ export class EuiAccordionClass extends Component<
'euiAccordion__children-isLoading': isLoading,
});

const buttonClasses = classNames(
'euiAccordion__button',
buttonClassName,
buttonPropsClassName
);

const buttonContentClasses = classNames(
'euiAccordion__buttonContent',
buttonContentClassName
);

const iconButtonClasses = classNames(
'euiAccordion__iconButton',
{
'euiAccordion__iconButton-isOpen': this.isOpen,
'euiAccordion__iconButton--right': _arrowDisplay === 'right',
},
arrowProps?.className
);

// Emotion styles
const buttonStyles = euiAccordionButtonStyles(theme);
const cssButtonStyles = [
buttonStyles.euiAccordion__button,
isDisabled && buttonStyles.disabled,
...(buttonPaddingSize
? [
buttonStyles[buttonPaddingSize],
arrowDisplay === 'left' && buttonStyles.arrowLeft,
arrowDisplay === 'right' && buttonStyles.arrowRight,
]
: []),
buttonPropsCss,
];

const childrenStyles = euiAccordionChildrenStyles(theme);
const cssChildrenStyles = [
childrenStyles.euiAccordion__children,
Expand All @@ -369,60 +315,9 @@ export class EuiAccordionClass extends Component<
this.isOpen && childWrapperStyles.isOpen,
];

const iconButtonStyles = euiAccordionIconButtonStyles(theme);
const cssIconButtonStyles = [
iconButtonStyles.euiAccordion__iconButton,
this.isOpen && iconButtonStyles.isOpen,
_arrowDisplay === 'right' && iconButtonStyles.arrowRight,
arrowProps?.css,
];

const optionalActionStyles = euiAccordionOptionalActionStyles();
const cssOptionalActionStyles = [
optionalActionStyles.euiAccordion__optionalAction,
];

const spinnerStyles = euiAccordionSpinnerStyles(theme);
const cssSpinnerStyles = [spinnerStyles.euiAccordion__spinner];

const triggerWrapperStyles = euiAccordionTriggerWrapperStyles();
const cssTriggerWrapperStyles = [
triggerWrapperStyles.euiAccordion__triggerWrapper,
];

let iconButton;
const buttonId = buttonProps.id ?? this.generatedId;
if (_arrowDisplay !== 'none') {
iconButton = (
<EuiButtonIcon
color="text"
{...arrowProps}
className={iconButtonClasses}
css={cssIconButtonStyles}
iconType="arrowRight"
onClick={this.onToggle}
aria-controls={id}
aria-expanded={this.isOpen}
aria-labelledby={buttonId}
tabIndex={buttonElementIsFocusable ? -1 : 0}
isDisabled={isDisabled}
/>
);
}

let optionalAction = null;

if (isLoading || extraAction) {
optionalAction = (
<div
className="euiAccordion__optionalAction"
css={cssOptionalActionStyles}
>
{isLoading ? <EuiLoadingSpinner /> : extraAction}
</div>
);
}

let childrenContent: any;
if (isLoading && isLoadingMessage) {
childrenContent = (
Expand All @@ -446,34 +341,26 @@ export class EuiAccordionClass extends Component<
childrenContent = children;
}

const button = (
<ButtonElement
{...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' ? this.isOpen : undefined}
onClick={isDisabled ? undefined : this.onToggle}
type={ButtonElement === 'button' ? 'button' : undefined}
disabled={ButtonElement === 'button' ? isDisabled : undefined}
>
<span className={buttonContentClasses}>{buttonContent}</span>
</ButtonElement>
);
const buttonId = buttonProps?.id ?? this.generatedId;

return (
<Element className={classes} css={cssStyles} {...rest}>
<div
className="euiAccordion__triggerWrapper"
css={cssTriggerWrapperStyles}
>
{_arrowDisplay === 'left' && iconButton}
{button}
{optionalAction}
{_arrowDisplay === 'right' && iconButton}
</div>
<EuiAccordionTrigger
ariaControlsId={id}
buttonId={buttonId}
// Force button element to be a legend if the element is a fieldset
buttonElement={Element === 'fieldset' ? 'legend' : buttonElement}
buttonClassName={buttonClassName}
buttonContent={buttonContent}
buttonContentClassName={buttonContentClassName}
buttonProps={buttonProps}
arrowProps={arrowProps}
arrowDisplay={arrowDisplay}
isDisabled={isDisabled}
isOpen={this.isOpen}
onToggle={this.onToggle}
extraAction={isLoading ? <EuiLoadingSpinner /> : extraAction}
/>

<div
className="euiAccordion__childWrapper"
Expand Down
31 changes: 31 additions & 0 deletions src/components/accordion/accordion_trigger/accordion_arrow.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { css } from '@emotion/react';

import { UseEuiTheme } from '../../../services';
import { logicalCSS } from '../../../global_styling';

export const euiAccordionArrowStyles = ({ euiTheme }: UseEuiTheme) => ({
euiAccordion__arrow: css`
flex-shrink: 0;
${logicalCSS('margin-right', euiTheme.size.xs)}
transform: rotate(
0deg
) !important; /* stylelint-disable-line declaration-no-important */
`,
isOpen: css`
transform: rotate(
90deg
) !important; /* stylelint-disable-line declaration-no-important */
`,
right: css`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it would be helpful to have an empty left key here for consistency? Left doesn't have any styles, but having the empty key would be a reminder that it's an option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure, I love it! I'll add it here in a bit

Copy link
Contributor Author

@cee-chen cee-chen Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There actually is CSS to put in the left styles! It avoids making right override left 🎉

dbcab02

This feels like a huge improvement, thank you for the awesome suggestion Bree!

${logicalCSS('margin-left', euiTheme.size.xs)}
${logicalCSS('margin-right', 0)}
`,
});
Loading