Skip to content

[EuiRange] Remove 20 tick hard limit, detect minimum tick width instead#6829

Merged
cee-chen merged 4 commits intoelastic:mainfrom
swapnil-talpade:fix/remove-tick-limit
Jun 6, 2023
Merged

[EuiRange] Remove 20 tick hard limit, detect minimum tick width instead#6829
cee-chen merged 4 commits intoelastic:mainfrom
swapnil-talpade:fix/remove-tick-limit

Conversation

@swapnil-talpade
Copy link
Copy Markdown
Contributor

@swapnil-talpade swapnil-talpade commented Jun 4, 2023 •
edited by cee-chen
Loading

Summary

closes #6807

This PR removes the hard-coded limit (thrown error) of 20 ticks visible, and instead uses the width of the track (on mount) to detect if each tick has a minimum width.

  • If each tick has 5px or less of width, the component throws/errors and will not render (same behavior as before when more than 20 ticks were visible).
  • If the component has between 5 to 20px of width available, the component will warn to the console, but not throw, and will still render
  • If each tick has more than 20px of width, no warning or error will be thrown.

screencap

QA

General checklist

  • Sign CLA
  • Checked in mobile (some warnings throw on the docs page, but no errors)
  • Updated documentation
  • Added or updated jest and cypress tests
  • A changelog entry exists and is marked appropriately
  • Checked for breaking changes and labeled appropriately

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 4, 2023

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link
Copy Markdown

cla-checker-service bot commented Jun 4, 2023 •
edited
Loading

💚 CLA has been signed

@swapnil-talpade swapnil-talpade marked this pull request as ready for review June 5, 2023 08:02
@cee-chen
Copy link
Copy Markdown
Contributor

cee-chen commented Jun 5, 2023

Heya @swapnil-talpade, thanks for taking on this PR! 💚 The code itself looks fine, but as a heads up, the CLA is complaining because you'll need to sign https://www.elastic.co/contributor-agreement with the email address specifically attached to the git commit (you can do a local git log to check for that email address).

Let me know when you've signed, and I'll push up a changelog and run CI tests after.

@swapnil-talpade
Copy link
Copy Markdown
Contributor Author

swapnil-talpade commented Jun 6, 2023

Thanks git log helped, turns out i was signing up with different email. I have signed with the one mentioned in git log hope this works.

@cee-chen cee-chen changed the title fix: remove 20 tick limit [EuiRange] Remove 20 tick hard limit Jun 6, 2023
cee-chen added 3 commits June 6, 2023 09:21
@cee-chen
Make warning copy more specific
f964fa4
@cee-chen
Make tick count detection semi-intelligent based on track width
352381e 
- more accurate and (hopefully) helpful than just using a more arbitrary # of ticks

- I was updating tests for the new behavior and couldn't help myself from going overboard
@cee-chen
Update docs and changelog
c8246b0
@cee-chen cee-chen added the breaking change PRs with breaking changes. (Don't delete - used for automation) label Jun 6, 2023
@cee-chen cee-chen changed the title [EuiRange] Remove 20 tick hard limit [EuiRange] Remove 20 tick hard limit, detect minimum tick width instead Jun 6, 2023
@cee-chen cee-chen mentioned this pull request Jun 6, 2023
Closed
@cee-chen
Copy link
Copy Markdown
Contributor

cee-chen commented Jun 6, 2023 •
edited
Loading

I went a little overboard when updating tests and figured I might as well make our tick render detection a tiny bit smarter while here 😄 The behavior now warns & errors based on track/tick width instead of a static tick count. I also marked this as a breaking change as it will potentially affect consumers who haven't been careful to test their components in smaller mobile screens. LMK if you have any questions about the changes!

@cee-chen
Copy link
Copy Markdown
Contributor

cee-chen commented Jun 6, 2023

jenkins test this

@cee-chen cee-chen enabled auto-merge (squash) June 6, 2023 18:17
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 6, 2023

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

@cee-chen cee-chen merged commit 1336267 into elastic:main Jun 6, 2023
@breehall breehall mentioned this pull request Jun 14, 2023
Merged
breehall added a commit to elastic/kibana that referenced this pull request Jun 14, 2023
@breehall
Upgrade EUI to v82.1.0 (#159744)
e82fa7f 
eui@81.3.0 ⏩ eui@82.1.0

## [`82.1.0`](https://github.com/elastic/eui/tree/v82.1.0)

- Added ability for `EuiMarkdownEditor` plugins to disable toolbar
buttons ([#6840](elastic/eui#6840))

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

**Bug fixes**

- Fixed `EuiPopover`'s types to omit `panelProps.hasBorder` and
`panelProps.hasShadow` - these props are not customizable on popovers
for visual consistency
([#6836](elastic/eui#6836))

**Breaking changes**

- `EuiRange` & `EuiDualRange` no longer have a hard limit of 20
displayed ticks. The component now instead detects the width available,
and throws an error if each tick has less than 5 pixels of width. We
recommend testing your tick usage at smaller screens to ensure they
always display legibly to users.
([#6829](elastic/eui#6829))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@cee-chen cee-chen cee-chen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

breaking change PRs with breaking changes. (Don't delete - used for automation)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[EuiRange] option to change/remove 20 tick limit

3 participants

@swapnil-talpade @kibanamachine @cee-chen