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

[Eui{Dual}Range] Rethink long labels at the boundaries #4781

Merged
merged 11 commits into from
May 14, 2021

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented May 5, 2021

Summary

Fixes #4765 by removing dynamic resizing of the range track based on label length, and introduces a new approach to shifting long labels that would otherwise overflow the margins of the components (EuiRange and EuiDualRange).

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile

  • Checked in Chrome, Safari, Edge, and Firefox

- [ ] Props have proper autodocs and playground toggles

  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@thompsongl
Copy link
Contributor Author

@miukimiu Does this seem reasonable to you from a design perspective? I'll take this out of draft mode if you are ok with the general approach.

@kibanamachine
Copy link

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

@miukimiu
Copy link
Contributor

miukimiu commented May 6, 2021

@thompsongl I've been testing the PR and there are a few things that I noticed.

When we have custom labels, is it possible to make them touch the start/end of the parent container? And the dots should touch the start/end of the track.

range-labels@2x

Another thing that I noticed is that sometimes the ticks are not created correctly. In the following examples, I was expecting to have a tick at the end of the track: https://gist.github.com/miukimiu/dc60d4f2658ac0a9bbfa695ae9f499de.

end-ticks@2x

@thompsongl
Copy link
Contributor Author

Thanks, @miukimiu!
Yes, custom labels intentionally had a spacing buffer on the end of the range. Not sure it's necessary any more, though.
Thanks for the gist so I can test the missing tick issue.

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

Opening this up for review. @miukimiu be aware that there are cases where the dots are not precisely at the end of the track because of percentage math, but those cases are now fewer and the extra space is now smaller.

@thompsongl thompsongl marked this pull request as ready for review May 11, 2021 20:06
@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented May 11, 2021

How are we thinking about the layout of these when no labels exist? It seems there's extra space that we could maybe get rid of in this case?

Screen Shot 2021-05-11 at 18 14 20 PM

@thompsongl
Copy link
Contributor Author

It seems there's extra space that we could maybe get rid of in this case?

Yep, that's good feedback

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

Thanks @thompsongl. It's looking good. 🎉

I just have a few suggestions/questions.

One the following example the labels are getting out of the container:

     <EuiDualRange
        fullWidth
        min={1619049600000}
        max={1619568000000}
        step={86400000}
        showTicks
        value={value2}
        onChange={onChange2}
      />

end-ticks-2@2x

But when we pass custom ticks they get inside of the container:

    <EuiDualRange
        fullWidth
        value={value2}
        onChange={onChange2}
        min={1619049600000}
        max={1619568000000}
        showTicks
        step={86400000}
        ticks={[
          { value: 1619049600000, label: '1619049600000' },
          { value: 1619136000000, label: '1619136000000' },
          { value: 1619222400000, label: '1619222400000' },
          { value: 1619308800000, label: '1619308800000' },
          { value: 1619395200000, label: '1619395200000' },
          { value: 1619481600000, label: '1619481600000' },
          { value: 1619568000000, label: '1619568000000' },
        ]}
      />

end-ticks-3@2x

Is this something that can be improved? So that the labels always get inside of the container?

src/components/form/range/_range_track.scss Outdated Show resolved Hide resolved
src/components/form/range/_range.scss Show resolved Hide resolved
@thompsongl
Copy link
Contributor Author

Is this something that can be improved? So that the labels always get inside of the container?

Not if we want to continue to allow text truncation. Label truncation only happens on non-custom labels, the assumption being that if a consumer provides custom labels, they want to see the whole label. The CSS method used to shift custom labels eliminates the ability to truncate text.
I don't recall clearly, but preventing container overflow may have been the reason why we added the dynamic resizing to begin with.

@thompsongl thompsongl requested a review from miukimiu May 13, 2021 15:09
@kibanamachine
Copy link

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

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

Thanks @thompsongl,
I tested again in Chrome, Safari, Edge and Firefox and LGTM! 🎉

@kibanamachine
Copy link

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

@thompsongl thompsongl merged commit 47b4e0c into elastic:master May 14, 2021
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.

[EuiDualRange] Bug when passing big integers as tick values
4 participants