-
Notifications
You must be signed in to change notification settings - Fork 860
[EuiPopover] Change hasArrow, position and offset defaults
#9218
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] Change hasArrow, position and offset defaults
#9218
Conversation
6d513e7 to
e72009c
Compare
|
LGTM @mgadewoll BTW the related Figma component was added to the new Borealis library and is using the same new defaults here |
@JoseLuisGJ Awesome, thanks a lot for the update! 🎉 |
@JoseLuisGJ I can see it on a Retina display in Chrome and only for the left aligned positions (
If we try to change the position further inwards, then other displays look not aligned.
I think it's somewhat of an edge case. |
|
Thanks Lene for the deep research on this rendering issue. This confirms that I was seeng this issue randomly, and it makes sense if it¡s related to the content width. I guess that's a trade off we can assume. |
acstll
left a comment
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.
🟢 Changes on the code side look correct and it's working as expected (checked Storybook following QA steps). Left a single non-blocking comment.
acstll
left a comment
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 did a second review going through the VRT updates, let me know what you think 🙂
packages/eui/.loki/reference/chrome_mobile_Layout_EuiPopover_EuiPopover_Playground.png
Show resolved
Hide resolved
packages/eui/.loki/reference/chrome_mobile_Layout_EuiWrappingPopover_Playground.png
Show resolved
Hide resolved
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.
maybe this looks better still centered?
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 should probably remain centered as well?
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.
Good question. Right now it follows the popover default.
I would agree that centered looks better, but let's get that also agreed on by design. Maybe @JoseLuisGJ could provide an opinion?
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.
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 agree too centered looks better, as you suggest, let's wait for @JoseLuisGJ's blessing 🙂
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 agree that initially might be following the Popover default offset, but in this case is more appropriate to center it completely IMO as well.
3970140 to
31c6626
Compare
- triggered by updated popover positioning
7d9abac to
2c1dfa7
Compare
💚 Build SucceededHistory
cc @mgadewoll |
💚 Build Succeeded
History
cc @mgadewoll |
- `@elastic/eui`: `v109.1.0` ⏩ `v109.2.0` - `@elastic/eui-theme-borealis`: `v5.0.0` ⏩ `v5.1.0` [Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams) --- ## Changes - Only snapshot updates related to EuiPopover, ToolTip and Table changes (see below) ## Package updates ### `@elastic/eui` v109.2.0 - Updated `EuiFlexItem` to fall back to `grow={true}` if invalid values for `grow` are passed ([#9228](elastic/eui#9228)) - Updated shared button styles in `useEuiButtonColorCSS` to use `euiDisabledSelector` ([#9226](elastic/eui#9226)) - Added `euiTextTruncateCSS` Emotion style utility ([#9231](elastic/eui#9231)) - Added `hasBackground` prop on `EuiTable`, `EuiBasicTable` and `EuiInMemoryTable` ([#9224](elastic/eui#9224)) - Added component token `components.tableFooterBackground` ([#9224](elastic/eui#9224)) - Updated the color of mobile table header cells to use `colors.textSubdued` ([#9224](elastic/eui#9224)) - Updated `EuiSuperDatePicker` to show a tooltip with the full range details when the button displays a pretty duration e.g. "Last 15 minutes" ([#9221](elastic/eui#9221)) - Updated `EuiPopover` default prop values of `hasArrow`, `position` and `offset`: ([#9218](elastic/eui#9218)) - Changed `hasArrow` to `false` - Changed `position` to `downLeft` - Changed `offset` to `4` when `hasArrow=false` - Updated `EuiInputPopover` `offset` default value to `2` ([#9218](elastic/eui#9218)) - Updated `EuiTourStep` to not apply `hasArrow=true` by default when `decoration="none"` ([#9218](elastic/eui#9218)) - Updated `EuiSuperDatePicker` to have a more forgiving manual input for absolute dates. ([#9199](elastic/eui#9199)) **Bug fixes** - Updated EuiButtonGroup disabled style selectors to use `euiDisabledSelector` to ensure high contrast mode styles apply correctly ([#9226](elastic/eui#9226)) - Updated `EuiSuperDatePicker` to ensure its pretty format button dates are truncated correctly ([#9231](elastic/eui#9231)) - Fixed a visual bug for mobile table action buttons that causes shifting positions when changing color mode ([#8231](elastic/eui#8231)) ([#9224](elastic/eui#9224)) **Accessibility** - Improved the navigation of sibling `EuiToolTip` anchor elements in NVDA browse mode by adding an `id` to ensure they are unique ([#9208](elastic/eui#9208)) ### `@elastic/eui-theme-borealis` v5.1.0 - Added component token `components.tableFooterBackground` ([#9224](elastic/eui#9224)) --------- Co-authored-by: Jorge Sanz <[email protected]> Co-authored-by: Lene Gadewoll <[email protected]>
- `@elastic/eui`: `v109.1.0` ⏩ `v109.2.0` - `@elastic/eui-theme-borealis`: `v5.0.0` ⏩ `v5.1.0` [Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams) --- ## Changes - Only snapshot updates related to EuiPopover, ToolTip and Table changes (see below) ## Package updates ### `@elastic/eui` v109.2.0 - Updated `EuiFlexItem` to fall back to `grow={true}` if invalid values for `grow` are passed ([elastic#9228](elastic/eui#9228)) - Updated shared button styles in `useEuiButtonColorCSS` to use `euiDisabledSelector` ([elastic#9226](elastic/eui#9226)) - Added `euiTextTruncateCSS` Emotion style utility ([elastic#9231](elastic/eui#9231)) - Added `hasBackground` prop on `EuiTable`, `EuiBasicTable` and `EuiInMemoryTable` ([elastic#9224](elastic/eui#9224)) - Added component token `components.tableFooterBackground` ([elastic#9224](elastic/eui#9224)) - Updated the color of mobile table header cells to use `colors.textSubdued` ([elastic#9224](elastic/eui#9224)) - Updated `EuiSuperDatePicker` to show a tooltip with the full range details when the button displays a pretty duration e.g. "Last 15 minutes" ([elastic#9221](elastic/eui#9221)) - Updated `EuiPopover` default prop values of `hasArrow`, `position` and `offset`: ([elastic#9218](elastic/eui#9218)) - Changed `hasArrow` to `false` - Changed `position` to `downLeft` - Changed `offset` to `4` when `hasArrow=false` - Updated `EuiInputPopover` `offset` default value to `2` ([elastic#9218](elastic/eui#9218)) - Updated `EuiTourStep` to not apply `hasArrow=true` by default when `decoration="none"` ([elastic#9218](elastic/eui#9218)) - Updated `EuiSuperDatePicker` to have a more forgiving manual input for absolute dates. ([elastic#9199](elastic/eui#9199)) **Bug fixes** - Updated EuiButtonGroup disabled style selectors to use `euiDisabledSelector` to ensure high contrast mode styles apply correctly ([elastic#9226](elastic/eui#9226)) - Updated `EuiSuperDatePicker` to ensure its pretty format button dates are truncated correctly ([elastic#9231](elastic/eui#9231)) - Fixed a visual bug for mobile table action buttons that causes shifting positions when changing color mode ([elastic#8231](elastic/eui#8231)) ([elastic#9224](elastic/eui#9224)) **Accessibility** - Improved the navigation of sibling `EuiToolTip` anchor elements in NVDA browse mode by adding an `id` to ensure they are unique ([elastic#9208](elastic/eui#9208)) ### `@elastic/eui-theme-borealis` v5.1.0 - Added component token `components.tableFooterBackground` ([elastic#9224](elastic/eui#9224)) --------- Co-authored-by: Jorge Sanz <[email protected]> Co-authored-by: Lene Gadewoll <[email protected]>
…ck on its content (#245162) ## Summary This PR fixes a UI issue with the `kbn-cell-actions` rendering in hover mode. A recent [EUI PR](#244032) made a change to the default `offset` value: ### Context ``` - Updated EuiPopover default prop values of hasArrow, position and offset: (elastic/eui#9218) - Changed hasArrow to false - Changed position to downLeft - Changed offset to 4 when hasArrow=false ``` This offset change ended up making our cell actions almost unusable, as the gap that is now present between the hovered content and the content of the `EuiPopover` is not 4 pixels (instead of previously 0). This means that when leaving the hovered content and before reaching the `EuiPopover` content, the popover is actually being removed... Before the EUI `109.2.0` commit https://github.com/user-attachments/assets/4ff1e2ef-38cc-486e-a236-1df400b2a5d0 Right at the EUI `109.2.0` commit https://github.com/user-attachments/assets/e4af2ca6-36fc-48e9-aee7-c8a9fc00ede3 ### Solution Add `offset={0}` to the `EuiPopover` in the `kbn-cell-actions` package. That way we do not have to change the UI and the correct behavior is restored. ### Checklist - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels.
- `@elastic/eui`: `v109.1.0` ⏩ `v109.2.0` - `@elastic/eui-theme-borealis`: `v5.0.0` ⏩ `v5.1.0` [Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams) --- ## Changes - Only snapshot updates related to EuiPopover, ToolTip and Table changes (see below) ## Package updates ### `@elastic/eui` v109.2.0 - Updated `EuiFlexItem` to fall back to `grow={true}` if invalid values for `grow` are passed ([elastic#9228](elastic/eui#9228)) - Updated shared button styles in `useEuiButtonColorCSS` to use `euiDisabledSelector` ([elastic#9226](elastic/eui#9226)) - Added `euiTextTruncateCSS` Emotion style utility ([elastic#9231](elastic/eui#9231)) - Added `hasBackground` prop on `EuiTable`, `EuiBasicTable` and `EuiInMemoryTable` ([elastic#9224](elastic/eui#9224)) - Added component token `components.tableFooterBackground` ([elastic#9224](elastic/eui#9224)) - Updated the color of mobile table header cells to use `colors.textSubdued` ([elastic#9224](elastic/eui#9224)) - Updated `EuiSuperDatePicker` to show a tooltip with the full range details when the button displays a pretty duration e.g. "Last 15 minutes" ([elastic#9221](elastic/eui#9221)) - Updated `EuiPopover` default prop values of `hasArrow`, `position` and `offset`: ([elastic#9218](elastic/eui#9218)) - Changed `hasArrow` to `false` - Changed `position` to `downLeft` - Changed `offset` to `4` when `hasArrow=false` - Updated `EuiInputPopover` `offset` default value to `2` ([elastic#9218](elastic/eui#9218)) - Updated `EuiTourStep` to not apply `hasArrow=true` by default when `decoration="none"` ([elastic#9218](elastic/eui#9218)) - Updated `EuiSuperDatePicker` to have a more forgiving manual input for absolute dates. ([elastic#9199](elastic/eui#9199)) **Bug fixes** - Updated EuiButtonGroup disabled style selectors to use `euiDisabledSelector` to ensure high contrast mode styles apply correctly ([elastic#9226](elastic/eui#9226)) - Updated `EuiSuperDatePicker` to ensure its pretty format button dates are truncated correctly ([elastic#9231](elastic/eui#9231)) - Fixed a visual bug for mobile table action buttons that causes shifting positions when changing color mode ([elastic#8231](elastic/eui#8231)) ([elastic#9224](elastic/eui#9224)) **Accessibility** - Improved the navigation of sibling `EuiToolTip` anchor elements in NVDA browse mode by adding an `id` to ensure they are unique ([elastic#9208](elastic/eui#9208)) ### `@elastic/eui-theme-borealis` v5.1.0 - Added component token `components.tableFooterBackground` ([elastic#9224](elastic/eui#9224)) --------- Co-authored-by: Jorge Sanz <[email protected]> Co-authored-by: Lene Gadewoll <[email protected]>
…ck on its content (elastic#245162) ## Summary This PR fixes a UI issue with the `kbn-cell-actions` rendering in hover mode. A recent [EUI PR](elastic#244032) made a change to the default `offset` value: ### Context ``` - Updated EuiPopover default prop values of hasArrow, position and offset: (elastic/eui#9218) - Changed hasArrow to false - Changed position to downLeft - Changed offset to 4 when hasArrow=false ``` This offset change ended up making our cell actions almost unusable, as the gap that is now present between the hovered content and the content of the `EuiPopover` is not 4 pixels (instead of previously 0). This means that when leaving the hovered content and before reaching the `EuiPopover` content, the popover is actually being removed... Before the EUI `109.2.0` commit https://github.com/user-attachments/assets/4ff1e2ef-38cc-486e-a236-1df400b2a5d0 Right at the EUI `109.2.0` commit https://github.com/user-attachments/assets/e4af2ca6-36fc-48e9-aee7-c8a9fc00ede3 ### Solution Add `offset={0}` to the `EuiPopover` in the `kbn-cell-actions` package. That way we do not have to change the UI and the correct behavior is restored. ### Checklist - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels.

















Summary
Closes #9184
This PR updates
EuiPopoverby changing the default values of these props to change the visual output of the component:hasArrowpositionoffsethasArrowbooleanfalsetruepositionuniondownLeftdownCenteroffsetnumberhasArrow?0:44Additionally,
EuiInputPopoveris updated by changing itsoffset.offsetnumber20EuiTourStepis updated to only sethasArrowon its underlyingEuiPopoverby default whendecoration="beacon"and notdecoration="none"(unless manually overridden).Why are we making this change?
🎨 UI refresh: The changes are part of the effort to refresh and modernize the UI (#9183)
Screenshots #
EuiPopover
EuiInputPopover
EuiTourStep with
decoration="none"Impact to users
🟢 No implementation updates required on consumer side.
QA
🧪 Storybook:
Checks:
EuiPopoverupdates are correct and that manually set props apply as expectedpositionisdownLeftoffsetis4withhasArrow=falseand0withhasArrow=0(same as production)EuiInputPopoverhas a defaultoffsetof2EuiTourStepwithdecoration="none"does not render an arrow by default (applyinghasArrowwill still override this)General checklist
@defaultif default values are missing) and playground toggles