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

Fix EuiProgress styles #3948

Merged
merged 11 commits into from
Aug 25, 2020
Merged

Conversation

andreadelrio
Copy link
Contributor

@andreadelrio andreadelrio commented Aug 20, 2020

Summary

I tried to implement the new EuiProgress props into Kibana (Lens/Discover sidebars) and run into some small styling issues. See:

Frame 62

What I'm proposing is to add an extra class for when euiProgress__data has a label to apply different styling.

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

@andreadelrio andreadelrio requested a review from cchaos August 20, 2020 22:30
@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Aug 21, 2020

The good thing is that I'm not seeing any differences between the old and new versions. Though I am slightly confused as to why you were getting such a different render in Kibana. The Kibana version looks like it just wasn't handling the truncation properly?

Could you copy/paste what the DOM render looks like on the Kibana side? Just to see what the configuration is over there and why it ended up so different. My main concern is just that negative margin. We've already seen this break other components like EuiFacetGroup.

@andreadelrio
Copy link
Contributor Author

andreadelrio commented Aug 24, 2020

Could you copy/paste what the DOM render looks like on the Kibana side? Just to see what the configuration is over there and why it ended up so different. My main concern is just that negative margin. We've already seen this break other components like EuiFacetGroup.

@cchaos Not sure if this is what you meant but

  1. Custom implementation using EuiFlexGroup and EuiFlexItem (what we currently have in Kibana). Progress + label/value in one FlexItem (which in turns contains a FlexGroup). Filter icons in another FlexItem (not shown in the screenshot below).
    image
    file: https://github.com/elastic/kibana/blob/master/src/plugins/discover/public/application/components/sidebar/discover_field_bucket.tsx#L51

  2. DOM render with current EuiProgress
    image

@andreadelrio
Copy link
Contributor Author

@cchaos thanks for your PR, just merged it. Just one question, what are tabular numbers?

Also I added margin-left: auto; back for valueText for the case when there are no labels.

image 44

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Aug 25, 2020

Essentially tabular numbers makes the numbers of a font behave almost like they are a monospace font so that when they are lined up in a column, they align better. Here's a screenshot of how it's supposed to behave with Inter. As I mentioned in the meeting, however, this is not currently working with the Google font. So I'll have to explore that later.

Screen Shot 2020-08-25 at 11 37 42 AM

Copy link
Contributor

@cchaos cchaos 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. I think this solution is he best situation we can get to while enforcing consumers to modify their values to ensure proper display.

@andreadelrio
Copy link
Contributor Author

@cchaos Thanks for the explanation! merging now.

@andreadelrio andreadelrio merged commit 399883b into elastic:master Aug 25, 2020
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