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

Update EuiDualRange to support passing more props to underlying range inputs #6902

Merged

Conversation

tkajtoch
Copy link
Member

@tkajtoch tkajtoch commented Jul 4, 2023

Summary

This PR updates EuiDualRange to support passing more props to underlying EuiRangeInput components using minInputProps and maxInputProps.

It's a fix for #6853 we came up with after a few iterations of possible solutions.

QA

General checklist

  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • A changelog entry exists and is marked appropriately

@tkajtoch tkajtoch self-assigned this Jul 4, 2023
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6902/

});
});

it('renders slider role elements as disabled when disabled=true', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the thoroughness with these tests, seriously! 🎉

Comment on lines +276 to +283
it('applies passed props to max input', () => {
const { container } = render(
Copy link
Contributor

Choose a reason for hiding this comment

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

[super optional, not a change request] would it be worth it to add a test checking that we allow overriding the default props? Something like

<EuiDualRange
  disabled
  maxInputProps={{ disabled: false }}
/>
// check that `maxInputProps` took precedence

@tkajtoch tkajtoch force-pushed the fix/6853-eui-dual-range-jumps-to-outer-values branch from 958c261 to 819f024 Compare July 10, 2023 16:32
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6902/

tkajtoch and others added 4 commits July 10, 2023 19:13
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Let's get this merged in!

FYI @Heenawter, since we discussed this a while back - this should be landing in Kibana hopefully end of this week or the next week :)

@tkajtoch tkajtoch enabled auto-merge (squash) July 10, 2023 17:45
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6902/

@tkajtoch
Copy link
Member Author

jenkins test this please

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6902/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6902/

cee-chen added a commit to elastic/kibana that referenced this pull request Jul 14, 2023
## Summary

`eui@83.1.0` ⏩ `eui@84.0.0`

---

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

- Updated `EuiDualRange`'s `minInputProps` and `maxInputProps` to
support passing more props to underlying inputs
([#6902](elastic/eui#6902))
- `EuiFocusTrap` now supports configuring cross-iframe focus trapping
via the `crossFrame` prop
([#6908](elastic/eui#6908))

**Bug fixes**

- Fixed `EuiFilterButton` icon display
([#6900](elastic/eui#6900))
- Fixed `EuiCombobox` compressed plain text display
([#6910](elastic/eui#6910))
- Fixed visual appearance of collapse buttons on collapsible
`EuiResizablePanel`s ([#6926](elastic/eui#6926))

**Breaking changes**

- `EuiFocusTrap` now defaults to *not* trapping focus across iframes
([#6908](elastic/eui#6908))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
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.

3 participants