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

[EuiPopover][EuiInputPopover] Misc fixes and CSS cleanup #7211

Merged
merged 11 commits into from
Sep 22, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Sep 21, 2023

Summary

As always, I recommend following along by commit because this PR has a bunch of CSS cleanups as well as fixes.

Missing close animations on standard EuiPopovers

This broke in EuiPopover's Emotion conversion (#5977)

Before (popovers disappear immediately on close) After (close should have a transition)
close_before close_after
dragdrop_before dragdrop_after

Fix EuiInputPopover flipping to horizontal anchors on screens without sufficient vertical space

This one's been broken since #7071 (my fault!)

Before After

QA

See above screenshots, feel free to repro on staging vs production

General checklist

  • Browser QA
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
      - [ ] Checked in both light and dark modes
      - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA - N/A, skipped
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.

+ convert `calc()` to `mathWithUnits`

+ fix missing opacity transition on drag/drop popovers
- that don't have enough enough vertical room to be top/bottom anchored
- has been broken since EuiPopover was converted to Emotion; was working with previous Sass

- the transition needs to be permanently present (not just on open) for it to still work on close
- by making our conditional JS cleaner, we don't have to constantly override existing tranform/filter CSS

+ clarify comments/var names
- now that the default styles no longer have any transforms, this offset isn't needed whatsoever for anchored/input popovers

+ clean up tests to use specific assertions
- EuiInputPopovers were anchoring to the left/right when they should not be
@cee-chen cee-chen marked this pull request as ready for review September 21, 2023 18:07
@cee-chen cee-chen requested a review from a team as a code owner September 21, 2023 18:07
@cee-chen cee-chen requested a review from 1Copenut September 21, 2023 18:07
@cee-chen
Copy link
Contributor Author

Assigned this to @1Copenut because I've been giving Bree a lot of PRs to review lately, but if you have some spare time and feel like this reviewing this feel free @breehall!

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.

👍 LGTM! I learned a new trick about using useMemo() to cache styles between re-renders.

@cee-chen
Copy link
Contributor Author

using useMemo() to cache styles between re-renders.

Just to add a bit more context here, perf wise the memoization is probably negligible, I wanted it more for the the early return syntax (which I personally find much easier to read than else ifs and also usually it's less of a typing headache). But yeah it's nice that we can get a little microperf boost while doing so!

arrowConfig: {
arrowWidth: 24,
arrowBuffer: 10,
},
returnBoundingBox: this.props.attachToAnchor,
allowCrossAxis: !this.props.attachToAnchor,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1Copenut FYI, I was thinking about this overnight and instead of tying this prop specifically to attachToAnchor, I think I'm going to make a new top-level EuiPopover prop for it and have EuiInputPopover (only) use the prop. There might be a scenario where attached anchors should flex to the cross axes, it's just that input popovers specifically should not

Copy link
Contributor

Choose a reason for hiding this comment

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

@cee-chen That sounds like a good use case to split this to a top level prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4420a7a

Skipped a dedicated docs example for it now as it feels relatively edge case 🤔 attachToAnchor doesn't have a docs section either fwiw, feels like this prop is similar in intention

@cee-chen cee-chen enabled auto-merge (squash) September 22, 2023 20:45
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen cee-chen merged commit 3a653d4 into elastic:main Sep 22, 2023
1 check passed
@cee-chen cee-chen deleted the popover-fixes branch September 22, 2023 21:15
jbudz pushed a commit to elastic/kibana that referenced this pull request Sep 27, 2023
`v88.3.0`⏩`v88.5.0`

closes #151514

---

## [`88.5.0`](https://github.com/elastic/eui/tree/v88.5.0)

- Updated `EuiCallOut` with a new `onDismiss` prop
([#7156](elastic/eui#7156))
- Added a new `renderCustomToolbar` prop to `EuiDataGrid`, which allows
custom rendering of the toolbar.
([#7190](elastic/eui#7190))
- Added a new `allowResetButton` prop to
`toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows
hiding the "Reset to default" button from the display settings popover.
([#7190](elastic/eui#7190))
- Added a new `additionalDisplaySettings` prop to
`toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows
rendering extra settings inside the display settings popover.
([#7190](elastic/eui#7190))
- Updated `EuiDataGrid`'s toolbar display settings button icon
([#7190](elastic/eui#7190))
- Updated `EuiTextTruncate` with significantly improved iteration
performance. Removed `measurementRenderAPI` prop, as `EuiTextTruncation`
now only uses more performant canvas render API
([#7210](elastic/eui#7210))
- Updated `EuiPopover` with a new configurable `repositionToCrossAxis`
prop ([#7211](elastic/eui#7211))
- Updated `EuiDatePicker` to support `compressed` input styling
([#7218](elastic/eui#7218))
- Added `gradient` and `palette` icon glyphs.
([#7220](elastic/eui#7220))

**Bug fixes**

- Fixed `EuiPopover`'s missing animations on popover close
([#7211](elastic/eui#7211))
- Fixed `EuiInputPopover` anchoring to the wrong side and missing
shadows on smaller screens
([#7211](elastic/eui#7211))
- Fixed `EuiSuperDatePicker` icon spacing on the quick select button
([#7217](elastic/eui#7217))
- Fixed a missing type in `EuiMarkdownEditor`'s default processing
plugins ([#7221](elastic/eui#7221))


## [`88.4.1`](https://github.com/elastic/eui/tree/v88.4.1)

**Bug fixes**

- Fixed missing `className`s on `EuiTextTruncate`
([#7212](elastic/eui#7212))
- Fixed `title`s on `EuiComboBox` dropdown options to always be present
([#7212](elastic/eui#7212))
- Fixed `EuiComboBox` truncation issues when search is an empty space
([#7212](elastic/eui#7212))

## [`88.4.0`](https://github.com/elastic/eui/tree/v88.4.0)

- Updated `EuiComboBox` to allow configuring text truncation behavior
via `truncationProps`. These props can be set on the entire combobox as
well as on on individual dropdown options.
([#7028](elastic/eui#7028))
- Updated `EuiInMemoryTable` with a new `searchFormat` prop (defaults to
`eql`). When setting this prop to `text`, the built-in search bar will
ignore EQL syntax and allow searching for plain strings with special
characters and symbols.
([#7175](elastic/eui#7175))

**Bug fixes**

- `EuiComboBox` now always shows the highlighted search text, even on
truncated text ([#7028](elastic/eui#7028))
- Fixed missing i18n in `EuiSearchBar`'s default placeholder and
aria-label text ([#7175](elastic/eui#7175))
- Fixed the inline compressed styles of `EuiDescriptionListTitle` to use
a taller line-height for readability
([#7185](elastic/eui#7185))
- Fixed `EuiComboBox` to correctly truncate selected items when
displayed as pills and plain text
([#7193](elastic/eui#7193))

**Accessibility**

- Added `aria-current` attribute to `EuiTablePagination`
([#7186](elastic/eui#7186))

**CSS-in-JS conversions**

- Converted `EuiDroppable` and `EuiDraggable` to Emotion; Removed
`$euiDragAndDropSpacing` Sass variables
([#7187](elastic/eui#7187))

---------

Co-authored-by: Patryk Kopycinski <[email protected]>
Co-authored-by: Jan Monschke <[email protected]>
Co-authored-by: Thomas Watson <[email protected]>
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