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

Jump to bottom

[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

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
28 changes: 13 additions & 15 deletions src/components/accordion/accordion.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,26 +10,26 @@ import React, { Component, HTMLAttributes, ReactNode } from 'react';
import classNames from 'classnames';
import { tabbable, FocusableElement } from 'tabbable';

import { CommonProps } from '../common';

import { EuiLoadingSpinner } from '../loading';
import { EuiResizeObserver } from '../observer/resize_observer';
import { EuiText } from '../text';
import { EuiI18n } from '../i18n';
import { logicalCSS } from '../../global_styling';
import {
htmlIdGenerator,
withEuiTheme,
WithEuiThemeProps,
} from '../../services';
import { CommonProps } from '../common';
import { EuiLoadingSpinner } from '../loading';
import { EuiResizeObserver } from '../observer/resize_observer';
import { EuiText } from '../text';
import { EuiI18n } from '../i18n';
import type { EuiButtonIconProps } from '../button';

import { EuiAccordionTrigger } from './accordion_trigger';
import {
euiAccordionStyles,
euiAccordionChildrenStyles,
euiAccordionChildWrapperStyles,
euiAccordionSpinnerStyles,
} from './accordion.styles';
import { logicalCSS } from '../../global_styling';

export const PADDING_SIZES = ['none', 'xs', 's', 'm', 'l', 'xl'] as const;
export type EuiAccordionPaddingSize = (typeof PADDING_SIZES)[number];
Expand Down Expand Up @@ -261,33 +261,31 @@ export class EuiAccordionClass extends Component<
render() {
const {
children,
buttonContent,
className,
id,
element: Element = 'div',
buttonElement,
buttonProps,
buttonClassName,
buttonContentClassName,
buttonContent,
arrowDisplay,
arrowProps,
Comment on lines +165 to +171
Copy link
Contributor

Choose a reason for hiding this comment

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

We should document the standard of "how to order imports" as a rough guideline in our How We Work wiki

Copy link
Member 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.

This is a super interesting (and potentially controversial 😅) topic of discussion actually. Personally, I work with the mental model of "group related props together", and it makes sense to me in my brain, but I've worked with devs in the past where that just doesn't make any sense to them and they strongly strictly prefer ordering EVERYTHING (imports, props, all object keys, etc.) alphabetically, and lint for it.

I'm going to be honest, I'm... not a huge fan of alphabetical sorting (there's just too many one-offs where related props end up far apart from each other, and it kinda relies on verbose or strict naming conventions). But yeah this is basically a topic where opinions can get a little heated and it can lead to a frustrating amount of bikeshedding, so all-in-all I've kinda just gone with the laissez-faire approach of "I'll clean it up if it's in my PR and I'm already here", as opposed to pushing for stricter linting around it.

It's definitely an interesting topic to discuss as a team (if it doesn't turn out into a spaces vs tabs holy war lol) so I'll put it on our next sync to quickly chat about if we're down for it!

extraAction,
paddingSize,
borders,
initialIsOpen,
arrowDisplay,
forceState,
isLoading,
isLoadingMessage,
isDisabled,
buttonProps,
buttonElement,
arrowProps,
theme,
...rest
} = this.props;

const classes = classNames(
'euiAccordion',
{
'euiAccordion-isOpen': this.isOpen,
},
{ 'euiAccordion-isOpen': this.isOpen },
className
);

Expand Down