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

[EuiFieldNumber] Fix native validity state when controlled value changes #7291

Merged
merged 7 commits into from
Oct 26, 2023
Merged

[EuiFieldNumber] Fix native validity state when controlled value changes #7291

merged 7 commits into from
Oct 26, 2023

Conversation

cee-chen
Copy link
Member

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

Summary

closes #7281

⚠️ NOTE: There is a slight gotcha to this PR in that our current version jsdom (11.x) incorrectly thinks that any input with a step passed to it is natively invalid. Upgrading to jsdom v15+ will fix this issue (and a few of our snapshots) in the future.

QA

  • Go to https://eui.elastic.co/pr_7291/storybook/?path=/story/euifieldnumber--controlled-component
  • Scroll down to the value prop control and change it to, e.g. 5
  • Change the min prop, to, e.g. 10
  • Confirm that the input correctly updates to show invalid state
  • Change the value prop to satisfy min, e.g. 100
  • Confirm the input correctly updates to no longer be invalid
  • (Optional) Test other props that would affect validity (e.g. max, step) as well to confirm that updating those props will correctly update the input's invalid state

General checklist

  • Browser QA
    • Checked in Chrome, Safari, Edge, and Firefox
  • Docs site QA - N/A
  • Code quality checklist
  • 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 - N/A

@cee-chen
Add E2E test demonstrating the problem/testing the solution
59d9927
@cee-chen
Call checkNativeValidity on passed value change
5adce93 
- requires setting up an internal input ref to use and combining with the consumer `inputRef`
@cee-chen
[misc] Add controlled onChange regression test
c81f2d8
@cee-chen
changelog
8aff254
@cee-chen
Add storybook for manual QA
17ee2a7
@cee-chen
Add more fixes for invalid states not updating as expected
cd3a9e2 
+ remove `onBlur` logic - the useEffect now handles that on mount, and blur behavior should no longer be necessary
@cee-chen
jsdom workaround
e588aa3 
- NOTE: EuiDualRange snapshots will just be slightly incorrect until we're on jsdom 15+ 🤷
@cee-chen cee-chen marked this pull request as ready for review October 26, 2023 19:02
@cee-chen cee-chen requested a review from a team as a code owner October 26, 2023 19:02
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

1Copenut
1Copenut approved these changes Oct 26, 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.

👍 LGTM. I left one question about a chore issue for JSDOM, but that's independent of the PR.

src/components/form/field_number/field_number.test.tsx
Comment on lines +35 to +38
// TODO: Restore this once we upgrade Jest/jsdom to v15+. Right now passing
// a `step` prop always leads to jsdom thinking that validity.valid is false
// @see https://github.com/jsdom/jsdom/issues/2288
// step={1}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a chore ticket in for this? If not, I can add one quick.

Copy link
Member Author

Choose a reason for hiding this comment

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

if you want to create one that'd be super awesome! 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

Or alternatively we can bogart #6813

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll add one and link it to #6813 in just a short.

@cee-chen cee-chen merged commit 393a91d into elastic:main Oct 26, 2023
8 checks passed
@cee-chen cee-chen deleted the field-number-fix branch October 26, 2023 20:20
This was referenced Oct 30, 2023
Merged
Closed
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))
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
4 participants
@cee-chen @kibanamachine @elasticmachine @1Copenut