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

[EuiDescriptionList] Update inline styles #5534

Merged
merged 14 commits into from
Jan 19, 2022

Conversation

andreadelrio
Copy link
Contributor

@andreadelrio andreadelrio commented Jan 12, 2022

Summary

  • Updated EuiDescriptionListTitle inline type styles by removing the border, increasing font weight, and adjusting the background color for better readibility.
  • Before, the compressed version of the inline type in Amsterdam didn't have enough space between lines, and that's been fixed

Frame 95

Checklist

@kibanamachine
Copy link

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

@snide
Copy link
Contributor

snide commented Jan 13, 2022

Don't tell my mom about my Quake 2 addiction being the reason for dropping out. This visually looks much cleaner and will help in Discover!

@hetanthakkar

This comment has been minimized.

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.

🙌 This is already a marked improvement, thank you for initiating this!

I have a few suggestions, some very nit-picky code stuffs, and I found some old style issues that would be great to fix in this PR.

Suggestions

While this was so in the legacy theme, I think we can actually improve on the inline styles by keeping (not overriding) some of the inherent EuiBadge styles. For instance, the regular description list really enhances the title using bold, dark text like our typical titles. However, in the inline, it so much more subtle to find the differences between the title and description since the text display is exactly the same just with a very light background.

I suggest removing the overrides for font-weight, color, and possibly border (I think the extra space it adds helps space it out for better readability.
Screen Shot 2022-01-13 at 09 54 16 AM

This way is still much more aligned visually with the non-inline version by keeping the boldness of the title text.


Old bugs

We should remove the following property:

Because it causes unnecessary breaks in words like:

Screen Shot 2022-01-13 at 09 42 07 AM

This was from very very long ago when EUI was KUI and before we had better ways to fix word-wrapping. If we think we need to enforce breaking words like this, instead of just deleting we could swap it out for the euiTextBreakWord mixin.

And on that note of word-wrapping, EuiBadge is set to truncate where the old version of the title would wrap. How do you think we want to handle this situation? Let it truncate or force wrapping?
Screen Shot 2022-01-13 at 09 58 01 AM

src/components/description_list/_description_list.scss Outdated Show resolved Hide resolved
src/components/description_list/_description_list.scss Outdated Show resolved Hide resolved
src/components/description_list/_description_list.scss Outdated Show resolved Hide resolved
src/components/description_list/_description_list.scss Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

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.

🙌 LGTM (with one more tiny suggestion on variable usage).

Sorry for the run around on how to resolve this one. This is definitely the simplest necessary approach. Thank you!

background: $euiColorLightestShade;
border: $euiBorderThin;
padding: 0 $euiSizeXS;
border-radius: $euiBorderRadius / 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

One last suggestion that I just caught. This variable will work for both themes and result as 4px and 2px.

Suggested change
border-radius: $euiBorderRadius / 2;
border-radius: $euiBorderRadiusSmall;

@kibanamachine
Copy link

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

@andreadelrio andreadelrio merged commit 93fd58e into elastic:main Jan 19, 2022
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.

5 participants