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.

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

[EuiProvider] Set up componentDefaults prop, context, & documentation #6923

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jul 7, 2023

⚠️ Please be aware this work is merging into a feature branch (in order to avoid non-valid documentation in main). This initial PR sets up some basic infrastructure and initial documentation, but the actual behavior is not yet wired up (i.e., will not work as promised in the docs example). I'm planning on opening individual PRs for each component that we plan to support defaults for initially (at least portals, pagination, and focus trap as a bonus).

Summary

This PR sets up the prop, context, and documentation for an <EuiProvider componentDefaults={}> API that be used so that certain components (to be discussed) can have various settings configured at a global level instead of needing to be set on every instance.

I recommend following along by commit for review, as the last commit adds some nice-to-have (but not required) logic for adding badges to sub-sections of pages.

QA

General checklist

- [ ] Checked in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Checked Code Sandbox works for any docs examples
- [ ] Checked for breaking changes and labeled appropriately
- [ ] Checked for accessibility including keyboard-only and screenreader modes
- [ ] Updated the Figma library counterpart

+ light tests - actual defaults/override tests should be written per-component
@cee-chen cee-chen force-pushed the feature/provider/component-defaults branch from 25f47e7 to 663b7ac Compare July 7, 2023 22:54
@cee-chen cee-chen changed the title [EuiProvider] Set up componentDefaults context + documentation [EuiProvider] Set up componentDefaults prop, context, & documentation Jul 7, 2023
@cee-chen cee-chen marked this pull request as ready for review July 7, 2023 23:00
@cee-chen cee-chen requested a review from a team July 7, 2023 23:00
Comment on lines +27 to +37
const badge = (isBeta || isNew) && (
<EuiBetaBadge label={isBeta ? 'Beta' : 'New'} color="accent" size="s" />
);

titleNode = (
<>
<EuiTitle>
<h2 id={id}>{title}</h2>
<h2 id={id}>
{title}
{badge && <>&emsp;{badge}</>}
</h2>
Copy link
Member Author

@cee-chen cee-chen Jul 7, 2023

Choose a reason for hiding this comment

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

Just want to note that I know this new beta/new badge logic for subsections (de3cfc2) is not terribly clean and is pretty repetitive with the existing beta/new badge logic, but I'm not bothering to get super DRY or fancy with it since we're migrating all our docs to EUI+ at some point in the future.

Comment on lines +28 to +29
// Declaring as a static const for reference integrity/reducing rerenders
const emptyDefaults = {};
Copy link
Member Author

@cee-chen cee-chen Jul 7, 2023

Choose a reason for hiding this comment

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

I'm not sure how necessary this is, but I recall = {} defaults in destructuring causing rerender/reinstantation issues previously, and I figure better safe than sorry with things like this that exist at the top level of the app.

Copy link
Member

@tkajtoch tkajtoch Jul 12, 2023

Choose a reason for hiding this comment

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

It should be fine here because createContext will be called only once when executing the contents of the file in runtime and props defaults will only execute when componentDefaults is initialized or updated with undefined value (see PropertyDestructuringAssignmentEvaluation section of EcmaScript specification)

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6923/

Comment on lines +14 to +17
/**
* Provide a global setting for EuiPortal's default insertion position.
*/
EuiPortal?: { insert: EuiPortalProps['insert'] };
Copy link
Member Author

Choose a reason for hiding this comment

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

I cribbed this prop doc copy from @y1j2x34's excellent community PR: https://github.com/elastic/eui/pull/6889/files#diff-a327a1618b19bc3a89edef20304da7b7dcbe3cf879362a00ea48d08864e719f9R67

But as always, feel free to leave copy suggestions!

@cee-chen cee-chen force-pushed the feature/provider/component-defaults branch from 663b7ac to 7728a18 Compare July 7, 2023 23:21
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6923/

@cee-chen
Copy link
Member Author

cee-chen commented Jul 8, 2023

@elastic/eui-team 👋 Hey y'all! I'm not assigning this PR to any one specific dev, but would love to hear from you all on this. @tkajtoch, I know you're busy with docsmobile work, but IIRC you commented on the original convo w/ Clint for this work - please let me know if you'd rather hold this PR until you've had a chance to review it.

BTW, crazy thought - part of me wonders how abstract we could get with this architecturally. Could we allow every component to obtain its defaults from this provider/context? 🤔 e.g., what if someone wanted all their EuiFlexGroups to have gapSize="s" and alignItems="center" and could just... pass that in to EuiProvider? 🤯

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

💯 This is a cool notion of global prop configuration. Referencing the community PR was a big help bridging the foundation work here and the end goal of adding the new Provider to EuiProvider.

I think we could extend component defaults, with a caveat I trend toward a conservative addition arc. Thinking out loud, if we ran a poll after the feature ships asking our community what components they'd like to see set globally, we might get a good sense where to put our energy.

/**
* Provide a global setting for EuiPortal's default insertion position.
*/
EuiPortal?: { insert: EuiPortalProps['insert'] };
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
EuiPortal?: { insert: EuiPortalProps['insert'] };
EuiPortal?: Partial<Pick<EuiPortalProps, 'insert'>>;

What do you think of doing this instead with the future goal of replacing it with just Partial<EuiPortalProps> when we're ready to support all props?

Copy link
Member Author

@cee-chen cee-chen Jul 12, 2023

Choose a reason for hiding this comment

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

I'm hesitant to expand this so soon, I think I'd rather stick to specific usages for now (per our beta callout) and expand it later when we consciously decide to.

If a consumer really wants to they could @ts-ignore and pass in other global EUI props in any case and that would work; so the support is technically there, but I don't want to be explicit about it just yet. Also, EuiPortal doesn't really have any other props (options are children and a ref function) that make sense to be configured globally vs per-instance in any case 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

@tkajtoch - just wanted to give you a heads up that I missed the Pick in this code suggestion and you were totally right on it 🤦 and I implemented it in a later PR here: c4bd1b1

@tkajtoch
Copy link
Member

I love to see this feature getting real! Thank you Cee for driving this effort.
I added a single comment about types but feel free to ignore it if you don't agree or don't want to spend time on typings right now. Other than that it's looking really solid 🎉

@cee-chen
Copy link
Member Author

cee-chen commented Jul 12, 2023

Thank you all for your feedback. Going to go ahead and merge this into the feature branch (assuming comments or lack of objections count as approvals, as this PR has been open for several days now).

There's still time to leave more feedback before the feature branch merges, so shout out whenever!

@cee-chen cee-chen merged commit e547a3c into elastic:feature/provider/component-defaults Jul 12, 2023
@cee-chen cee-chen deleted the feature/provider/component-defaults branch July 12, 2023 19:18
cee-chen added a commit that referenced this pull request Jul 12, 2023
…on (#6923)

* Set up `EuiComponentDefaultsProvider`

+ light tests - actual defaults/override tests should be written per-component

* Add documentation

* Add support for beta/new badges to subitems in nav and subheadings

* changelog
cee-chen added a commit to cee-chen/eui that referenced this pull request Jul 25, 2023
…on (elastic#6923)

* Set up `EuiComponentDefaultsProvider`

+ light tests - actual defaults/override tests should be written per-component

* Add documentation

* Add support for beta/new badges to subitems in nav and subheadings

* changelog
cee-chen added a commit to cee-chen/eui that referenced this pull request Jul 25, 2023
…on (elastic#6923)

* Set up `EuiComponentDefaultsProvider`

+ light tests - actual defaults/override tests should be written per-component

* Add documentation

* Add support for beta/new badges to subitems in nav and subheadings

* changelog
cee-chen added a commit to cee-chen/eui that referenced this pull request Jul 26, 2023
…on (elastic#6923)

* Set up `EuiComponentDefaultsProvider`

+ light tests - actual defaults/override tests should be written per-component

* Add documentation

* Add support for beta/new badges to subitems in nav and subheadings

* changelog
cee-chen added a commit that referenced this pull request Aug 1, 2023
…on (#6923)

* Set up `EuiComponentDefaultsProvider`

+ light tests - actual defaults/override tests should be written per-component

* Add documentation

* Add support for beta/new badges to subitems in nav and subheadings

* changelog
cee-chen added a commit to cee-chen/eui that referenced this pull request Aug 1, 2023
…on (elastic#6923)

* Set up `EuiComponentDefaultsProvider`

+ light tests - actual defaults/override tests should be written per-component

* Add documentation

* Add support for beta/new badges to subitems in nav and subheadings

* changelog
cee-chen added a commit to cee-chen/eui that referenced this pull request Aug 3, 2023
…on (elastic#6923)

* Set up `EuiComponentDefaultsProvider`

+ light tests - actual defaults/override tests should be written per-component

* Add documentation

* Add support for beta/new badges to subitems in nav and subheadings

* changelog
cee-chen added a commit to elastic/kibana that referenced this pull request Aug 21, 2023
`v86.0.0`⏩`v87.1.0`

⚠️ The biggest set of type changes in this PR come from the breaking
change that makes `pageSize` and `pageSizeOptions` now optional props
for `EuiBasicTable.pagination`, `EuiInMemoryTable.pagination` and
`EuiDataGrid.pagination`.

This caused several other components that were cloning EUI's pagination
type to start throwing type warnings about `pageSize` being optional.
Where I came across these errors, I modified the extended types to
require `pageSize`. These types and their usages may end up changing
again in any case once the Shared UX team looks into
#56406.

---

## [`87.1.0`](https://github.com/elastic/eui/tree/v87.1.0)

- Updated the underlying library powering `EuiAutoSizer`. This primarily
affects typing around the `disableHeight` and `disableWidth` props
([#6798](elastic/eui#6798))
- Added new `EuiAutoSize`, `EuiAutoSizeHorizontal`, and
`EuiAutoSizeVertical` types to support `EuiAutoSizer`'s now-stricter
typing ([#6798](elastic/eui#6798))
- Updated `EuiDatePickerRange` to support `compressed` display
([#7058](elastic/eui#7058))
- Updated `EuiFlyoutBody` with a new `scrollableTabIndex` prop
([#7061](elastic/eui#7061))
- Added a new `panelMinWidth` prop to `EuiInputPopover`
([#7071](elastic/eui#7071))
- Added a new `inputPopoverProps` prop for `EuiRange`s and
`EuiDualRange`s with `showInput="inputWithPopover"` set
([#7082](elastic/eui#7082))

**Bug fixes**

- Fixed `EuiToolTip` overriding instead of merging its
`aria-describedby` tooltip ID with any existing `aria-describedby`s
([#7055](elastic/eui#7055))
- Fixed `EuiSuperDatePicker`'s `compressed` display
([#7058](elastic/eui#7058))
- Fixed `EuiAccordion` to remove tabbable children from sequential
keyboard navigation when the accordion is closed
([#7064](elastic/eui#7064))
- Fixed `EuiFlyout`s to accept custom `aria-describedby` IDs
([#7065](elastic/eui#7065))

**Accessibility**

- Removed the default `dialog` role and `tabIndex` from push
`EuiFlyout`s. Push flyouts, compared to overlay flyouts, require manual
accessibility management.
([#7065](elastic/eui#7065))

## [`87.0.0`](https://github.com/elastic/eui/tree/v87.0.0)

- Added beta `componentDefaults` prop to `EuiProvider`, which will allow
configuring certain default props globally. This list of components and
defaults is still under consideration.
([#6923](elastic/eui#6923))
- `EuiPortal`'s `insert` prop can now be configured globally via
`EuiProvider.componentDefaults`
([#6941](elastic/eui#6941))
- `EuiFocusTrap`'s `crossFrame` and `gapMode` props can now be
configured globally via `EuiProvider.componentDefaults`
([#6942](elastic/eui#6942))
- `EuiTablePagination`'s `itemsPerPage`, `itemsPerPageOptions`, and
`showPerPageOptions` props can now be configured globally via
`EuiProvider.componentDefaults`
([#6951](elastic/eui#6951))
- `EuiBasicTable`, `EuiInMemoryTable`, and `EuiDataGrid` now allow
`pagination.pageSize` to be undefined. If undefined, `pageSize` defaults
to `EuiTablePagination`'s `itemsPerPage` component default.
([#6993](elastic/eui#6993))
- `EuiBasicTable`, `EuiInMemoryTable`, and `EuiDataGrid`'s
`pagination.pageSizeOptions` will now fall back to
`EuiTablePagination`'s `itemsPerPageOptions` component default.
([#6993](elastic/eui#6993))
- Updated `EuiHeaderLinks`'s `gutterSize` spacings
([#7005](elastic/eui#7005))
- Updated `EuiHeaderAlert`'s stacking styles
([#7005](elastic/eui#7005))
- Added `toolTipProps` to `EuiListGroupItem` that allows customizing
item tooltips. ([#7018](elastic/eui#7018))
- Updated `EuiBreadcrumbs` to support breadcrumbs that toggle popovers
via `popoverContent` and `popoverProps`
([#7031](elastic/eui#7031))
- Improved the contrast ratio of disabled titles within `EuiSteps` and
`EuiStepsHorizontal` to meet WCAG AA guidelines.
([#7032](elastic/eui#7032))
- Updated `EuiSteps` and `EuiStepsHorizontal` to highlight and provide a
more clear visual indication of the current step
([#7048](elastic/eui#7048))

**Bug fixes**

- Single uses of `<EuiHeaderSectionItem side="right" />` now align right
as expected without needing a previous `side="left"` sibling.
([#7005](elastic/eui#7005))
- `EuiPageTemplate` now correctly displays `panelled={true}`
([#7044](elastic/eui#7044))

**Breaking changes**

- `EuiTablePagination`'s default `itemsPerPage` is now `10` (was
previously `50`). This can be configured through
`EuiProvider.componentDefaults`.
([#6993](elastic/eui#6993))
- `EuiTablePagination`'s default `itemsPerPageOptions` is now `[10, 25,
50]` (was previously `[10, 20, 50, 100]`). This can be configured
through `EuiProvider.componentDefaults`.
([#6993](elastic/eui#6993))
- Removed `border` prop from `EuiHeaderSectionItem` (unused since
Amsterdam theme) ([#7005](elastic/eui#7005))
- Removed `borders` object configuration from `EuiHeader.sections`
([#7005](elastic/eui#7005))

**CSS-in-JS conversions**

- Converted `EuiHeaderAlert` to Emotion; Removed unused
`.euiHeaderAlert__dismiss` CSS
([#7005](elastic/eui#7005))
- Converted `EuiHeaderSection`, `EuiHeaderSectionItem`, and
`EuiHeaderSectionItemButton` to Emotion
([#7005](elastic/eui#7005))
- Converted `EuiHeaderLinks` and `EuiHeaderLink` to Emotion; Removed
`$euiHeaderLinksGutterSizes` Sass variables
([#7005](elastic/eui#7005))
- Removed `$euiHeaderBackgroundColor` Sass variable; use
`$euiColorEmptyShade` instead
([#7005](elastic/eui#7005))
- Removed `$euiHeaderChildSize` Sass variable; use `$euiSizeXXL` instead
([#7005](elastic/eui#7005))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants