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

[EuiRangeLevels] Align ticks and levels #5181

Merged
merged 6 commits into from
Oct 4, 2021

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Sep 14, 2021

Summary

Waiting for #5171 to merge so we have relevant docs

Closes #5172. Aligns the gaps between levels and associated ticks. Alignment may still be off by 1px or so, but much improved.

image

Note that the snapshot tests are less relevant now because we rely on calculable width, which jsdom does not do well.
We should look to Cypress for better tests.

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 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

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Sep 14, 2021

I probably should have mentioned this in the issue before work got started, but I'd like to take a step back to "redesign" this idea of levels. The main problem is that we have a min/max value to provide for each, which means that the levels never really encompass that number (not visually anyhow). This can be confusing if the consumer did not also provide extra context.

Image 2021-09-14 at 4 18 19 PM

So the question is, is this just an implementation detail and we need to change the docs so that the custom ticks live within the levels or do we find a way to ensure they overlap the values provided (even if the values literally overlap, i.e. max: 25, min: 25?

And then just a general note about "design"... Should we consider replacing the track with the levels instead of trying to show beneath it? At least in the single range version?

Screen Shot 2021-09-14 at 16 28 20 PM

@thompsongl
Copy link
Contributor Author

I probably should have mentioned this in the issue before work got started, but I'd like to take a step back to "redesign" this idea of levels

I'm glad you brought this up and there's no rush (IMO) to get the current bug fixed. So I can pause this for the moment.
The problem we have is we don't indicate whether the level ranges are inclusive or exclusive. The design would be simpler if conceptually we knew that min was inclusive and max was exclusive (or whatever; this is my interpretation of how it wors now). The gap between levels was an easy if naive way to get around having to define inclusiveness.

@cchaos
Copy link
Contributor

cchaos commented Sep 14, 2021

Yep! I totally agree that the ambiguity around inclusivity is the main issue here. We could establish that min is always inclusive which in turn makes max optional. So you could write this markup:

  const levels = [
    {
      min: 0,
      max: 25,
      color: 'primary',
    },
    {
      min: 25,
      max: 50,
      color: 'success',
    },
    {
      min: 50,
      max: 90,
      color: 'warning',
    },
    {
      min: 90,
      max: 100,
      color: 'danger',
    },
  ];

Similarly as:

  const levels = [
    {
      min: 0,
      color: 'primary',
    },
    {
      min: 25,
      color: 'success',
    },
    {
      min: 50,
      color: 'warning',
    },
    {
      min: 90,
      color: 'danger',
    },
  ];

Where each level then starts at that minimum making the previous one (if exists) stop 1 step prior. Leaving max values to establish ending values if there is no consecutive level or to create gaps. For example something like:

  const levels = [
    {
      min: 0,
      color: 'primary',
    },
    {
      min: 25,
      color: 'success',
    },
    {
      min: 50,
      max: 60,
      color: 'warning',
    },
    {
      min: 90,
      max: 95,
      color: 'danger',
    },
  ];

Would look something like:

image

@cchaos
Copy link
Contributor

cchaos commented Sep 15, 2021

Or honestly just looking at that design, the levels bar could really just reuse the color stops component instead of trying to chunk it. That may also make the calculations easier?

@miukimiu
Copy link
Contributor

IMO we should merge this fix once #5171 is merged. As we can see here, the levels don't look good and we're just fixing what consumers are currently using.

Screenshot 2021-09-23 at 12 32 07

After that, we can redesign or reuse the color stops but I guess this will take time and for now, we just make the levels alignment make more sense.

Let me know if you both agree in merging this PR.

@thompsongl
Copy link
Contributor Author

thompsongl commented Sep 27, 2021

I'd be ok with merging this patch and rethinking the design in a separate effort, especially if we want to explore the idea to "reuse the color stops component" or alter the DOM in a significant way.

@thompsongl thompsongl marked this pull request as ready for review September 30, 2021 14:50
@kibanamachine
Copy link

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

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,

Tested in Chrome, Safari, Edge and Firefox, and LGTM! 🎉

@thompsongl thompsongl enabled auto-merge (squash) October 4, 2021 14:26
@kibanamachine
Copy link

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

@thompsongl thompsongl merged commit 43dec8b into elastic:master Oct 4, 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.

[Eui(Dual)Range] Levels not positioning correctly
4 participants