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

[EuiDualRange] Fix buggy highlight positioning #7305

Merged
merged 6 commits into from
Oct 25, 2023
Merged

[EuiDualRange] Fix buggy highlight positioning #7305

merged 6 commits into from
Oct 25, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Oct 23, 2023 •
edited by 1Copenut
Loading

Summary

closes #7304 - this behavior broke in #7241. Something in the EuiInputPopover refactors caused a rerender race condition, where the ref pointing at content within the popover is now returning a smaller width than expected. Adding a requestAnimationFrame tick ensures that the width we receive from the ref is the correct/expected width, and fixes the bug (526d3c7).

Before After
screencap

⚠️ I strongly recommend reviewing by commit and turning off whitespace changes when viewing diffs, as there's some other fixes and misc setup/cleanup along the way.

QA

  • Go to https://eui.elastic.co/pr_7305/#/forms/range-sliders#inputs-with-range-in-a-dropdown
  • Click on the numeric inputs in the second set of inputs and confirm that the highlights display as expected visually, and do not look buggy
  • Regression testing
    • Click on the first single numeric input and confirm that that also looks/behaves visually as expected
    • Quickly smoke test that all other range sliders on the page look and behave as expected
  • Responsive testing
    • Open your browser devtools, and enter responsive mode
    • Smoke test that all range sliders (inside and out of popovers) re-position dynamically as the total range width increases or decreases

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
  • Code quality checklist
    • Added or updated jest and cypress tests ⚠️ NOTE: Unfortunately the dual range behavior is too DOM-dependent to write Jest tests for. We should opt to instead add visual regression tests in the future 🤞
  • Release checklist
    • A changelog entry exists and is marked appropriately.
      - [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist
    - [ ] Updated the Figma library counterpart

@cee-chen
[EuiInputPopover] Fix missing onPanelResize callback
20a0213
@cee-chen
[misc] Type fixes/cleanup
9c7411c 
- optional / not related to main fix
@cee-chen
[EuiRangeTrack][setup] Remove trackWidth ref
399a9a1 
- in favor of tracking width state from parent component and passing it down as a prop

- makes tests easier to mock as well
@cee-chen
[EuiRangeTrack] Remove children render prop
3f09ca1 
- `EuiDualRange` was already tracking a width state via the input ref, so all we need to do is pass it down to `EuiRangeTrack`
- `EuiRange` needs to be updated to add a new state
@cee-chen
[EuiDualRange] Fix incorrect width calculations caused by race condition
526d3c7 
- this is the ultimate fix for the opened issue - the conversion to a function component apparently is causing race conditions for the `ref` behavior - wrapping the `.clientWidth` call in a rAF fixes the issue

+ bonus - DRY out `this.props.showInput === 'inputWithPopover'` logic with a getter
@cee-chen
[EuiDualRange] Fix incorrect onResize input popover logic
b00f0bc 
- the `width` being passed is the total width of the popover, we want the range track only
@cee-chen cee-chen changed the title [EuiDualRange] Fix buggy [EuiDualRange] Fix buggy highlight positioning Oct 23, 2023
@cee-chen cee-chen marked this pull request as ready for review October 23, 2023 22:12
@cee-chen cee-chen requested a review from a team as a code owner October 23, 2023 22:12
@cee-chen
Copy link
Member Author

buildkite test this

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

1Copenut
1Copenut approved these changes Oct 25, 2023
View reviewed changes
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.

👍 The change looked good on all four evergreen browsers.

@cee-chen cee-chen merged commit cca5099 into elastic:main Oct 25, 2023
2 checks passed
@cee-chen cee-chen deleted the dual-range/fix branch October 25, 2023 15:14
@cee-chen cee-chen mentioned this pull request Oct 30, 2023
Merged
cee-chen added a commit to elastic/kibana that referenced this pull request Nov 3, 2023
@cee-chen
Upgrade EUI to v90.0.0 (#170179)
d079e95 
`v89.1.0`⏩`v90.0.0`

The majority of changes in this PR come from:

- **EuiContextMenu** being converted to Emotion
(elastic/eui#7312). If your usage of
`EuiContextMenu` was significantly affected, we recommend pulling down
this PR and QAing it locally.

- `defaultProps` being removed from some very widespread components,
particularly **EuiButton**, in anticipation of React's upcoming
deprecation.
(elastic/eui@b7dc9b4)
**NOTE**: This only affected Enzyme snapshots, and did not affect
production behavior.

[Commits](https://github.com/elastic/kibana/pull/170179/commits) have
been broken up by component changes as well as types of changes.

---

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

- Updated the `eventColor` prop on `EuiCommentEvent` to apply the color
to the entire comment header.
([#7288](elastic/eui#7288))
- Updated `EuiBasicTable` and `EuiInMemoryTable` to support a new
controlled selection API: `selection.selected`
([#7321](elastic/eui#7321))

**Bug fixes**

- Fixed controlled `EuiFieldNumbers` not correctly updating native
validity state ([#7291](elastic/eui#7291))
- Fixed `EuiListGroupItem` to pass `style` props to the wrapping `<li>`
element alongside `className` and `css`. All other props will be passed
to the underlying content.
([#7298](elastic/eui#7298))
- Fixed `EuiListGroupItem`'s non-transitioned transform on hover/focus
([#7298](elastic/eui#7298))
- Fixed `EuiDataGrid`s with `gridStyle.stripes` sometimes showing buggy
row striping after being sorted
([#7301](elastic/eui#7301))
- Fixed `EuiDataGrid`'s `gridStyle.rowClasses` API to not conflict with
`gridStyle.stripes` if dynamically updated
([#7301](elastic/eui#7301))
- Fixed `EuiDataGrid`'s `gridStyle.rowClasses` API to support multiple
space-separated classes
([#7301](elastic/eui#7301))
- Fixed `EuiInputPopover` not calling `onPanelResize` callback prop
([#7305](elastic/eui#7305))
- Fixed `EuiDualRange` incorrectly positioning highlights when rendered
with `showInput="inputWithPopover"`
([#7305](elastic/eui#7305))
- Fixed `EuiTabs` incorrectly wrapping text when it should instead
either scroll or truncate
([#7309](elastic/eui#7309))
- `EuiContextMenu` now renders text colors correctly when used within an
`EuiBottomBar` ([#7312](elastic/eui#7312))
- Fixed the width of `EuiSuperDatePicker`'s Absolute date picker
([#7313](elastic/eui#7313))
- Fixed `EuiDataGrid` cells visually cutting off overflowing content a
little too quickly ([#7320](elastic/eui#7320))

**Deprecations**

- Deprecated `EuiBasicTable` and `EuiInMemoryTable`'s ref `setSelection`
API. Use the new `selection.selected` API instead.
([#7321](elastic/eui#7321))

**Breaking changes**

- Removed `EuiPageTemplate_Deprecated`, `EuiPageSideBar_Deprecated`, and
`EuiPageContent*_Deprecated`
([#7265](elastic/eui#7265))
- Removed the `ghost` color option from `EuiButton`, `EuiButtonEmpty`,
and `EuiButtonIcon`. Use an `<EuiThemeProvider colorMode="dark">`
wrapper and `color="text"` instead.
([#7296](elastic/eui#7296))

**Dependency updates**

- Updated `refractor` to v3.6.0
([#7127](elastic/eui#7127))
- Updated `rehype-raw` to v5.1.0
([#7127](elastic/eui#7127))
- Updated `vfile` to v4.2.1
([#7127](elastic/eui#7127))

**Accessibility**

- `EuiContextMenu` now correctly respects reduced motion preferences
([#7312](elastic/eui#7312))
- `EuiAccordion`s no longer attempt to focus child content when the
accordion is externally opened via `forceState`, but will continue to
focus expanded content when users click the toggle button.
([#7314](elastic/eui#7314))

**CSS-in-JS conversions**

- Converted `EuiContextMenu`, `EuiContextMenuPanel`, and
`EuiContextMenuItem` to Emotion; Removed `$euiContextMenuWidth`
([#7312](elastic/eui#7312))
@cee-chen cee-chen mentioned this pull request Jan 3, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1Copenut 1Copenut 1Copenut approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[EuiDualRange] Rendering incorrectly in popover
4 participants
@cee-chen @kibanamachine @elasticmachine @1Copenut