-
Notifications
You must be signed in to change notification settings - Fork 826
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
[EuiInputPopover] Add panelMinWidth
prop
#7071
Conversation
- use a resizable textarea instead of width state - toggle fns make no sense
+ misc cleanup - optional chaining syntax
- needs to be cypress as jsdom can't process `getBoundingClientRect`
- by removing forced left-aligned `attachToAnchor` from EuiPopover + adjust types of `EuiInputPopover` to only accept the down positions
db1862b
to
0ab6197
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_7071_buildkite/ |
- Describe width behavior + note new `minWidth` prop - Enhance `minWidth` demo to allow showing the different behaviors combined with `anchorPosition` + key down behavior mentioned in description
0ab6197
to
893bdbf
Compare
893bdbf
to
7bacd72
Compare
@@ -145,7 +145,7 @@ exports[`EuiSuperSelect props custom display is propagated to dropdown 1`] = ` | |||
class="euiPanel euiPanel--plain euiPopover__panel emotion-euiPanel-grow-m-plain-euiPopover__panel-bottom" | |||
data-popover-panel="true" | |||
role="dialog" | |||
style="top: 8px; left: 0px; will-change: transform, opacity; z-index: 2000;" | |||
style="top: 8px; left: -22px; will-change: transform, opacity; z-index: 2000;" |
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.
I'm not totally sure why this snapshot changed - I QA'd all components dogfooding EuiInputPopover
(EuiComboBox, EuiSuperSelect, EuiRange, and EuiAutoRefresh) and all of them are still rendering the correct widths/positions for the popover. I'll put this down as Jest/jsdom gremlins 🤷
Preview documentation changes for this PR: https://eui.elastic.co/pr_7071_buildkite/ |
- was off by 2.5px on headless vs headed, so tweak widths so it stops doing that + explicitly set viewport width (in case the default changes in the future)
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.
Tested this in Kibana, and it does exactly what we need it to do for the options list control - thanks so much for tackling this so quickly! 🎉
Quick question though - would it be possible to make this prop available for the EuiDualRange
component for when showInput="inputWithPopover"
? We use this for our range slider control and, while less important than with the options list control, it would still be valuable to make the popover extend past the input on our smaller sizes:
@Heenawter I was wondering if EuiRange/DualRange would need this! Yes, I can certainly add a new prop that allows consumers to pass popover props to those components. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_7071_buildkite/ |
💚 Build Succeeded
History
cc @cee-chen |
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.
This is a great improvement to the input popover. Tested in staging and the panelMinWidth
prop and the anchorPosition
were both respected. I pulled this branch and ran the Cypress tests locally. They passed, so I think CI may need to be kicked off again for the tests.
Functionally, consumers have control over what events open and close | ||
the popover, and it can allow for natural tab order. Although some | ||
assumptions are made about keyboard behavior, consumers should | ||
provide specific key event handlers depending on the use case. For | ||
instance, a <EuiCode>type=text</EuiCode> input could use the down | ||
key to trigger popover opening, but this interaction would not be | ||
appropriate for <EuiCode>type=number</EuiCode> inputs as they | ||
natively bind to the down key. |
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.
Great explanation here. This is super informative!
/** | ||
* Alignment of the popover relative to the input | ||
*/ | ||
anchorPosition?: 'downLeft' | 'downRight' | 'downCenter'; |
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.
This is just a thought, no action required. I like the decision you made here to just set the prop to equal one of these three values instead of creating a separate type (which the pattern we're currently using in most components). Especially since there are only three values being used.
// Cypress doesn't seem to have a way to mimic manual dragging/resizing, so we'll do it programmatically | ||
cy.get('textarea').then(($el) => ($el[0].style.width = '500px')); | ||
cy.get('[data-popover-panel]').should('have.css', 'left', '50px'); | ||
}); |
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.
👍🏾
@cee-chen thank you so much for the quick turnaround on this. |
Thanks a ton all! @Heenawter, as a heads up, since Bree's already reviewed this, I'll go ahead and merge it in and add edit: follow-up PR: #7082 |
`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>
Summary
See this conversation elastic/kibana#162651 (comment) with @Heenawter and @andreadelrio about some of the new Discover work being done to standardize the visual behavior of input popovers.
Prior to this prop,
EuiInputPopover
widths were entirely tied to the width of the input element. Now, consumers can specify a minimum width, allowing wider content to display at desired widths if necessary, e.g.:This PR also fixes
EuiPopver
to allowEuiInputPopover
to respect different side anchors and to update the popover position if the input resizes.QA
General checklist
@default
if default values are missing)and playground togglesjest andcypress tests- [ ] Checked in both light and dark modes- [ ] Checked in mobile- [ ] Checked Code Sandbox works for any docs examples- [ ] Updated the Figma library counterpart- [ ] Checked for breaking changes and labeled appropriately