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] Add option for vertical spacing #5834

Closed
hbharding opened this issue Apr 21, 2022 · 8 comments · Fixed by #6175
Closed

[EuiDescriptionList] Add option for vertical spacing #5834

hbharding opened this issue Apr 21, 2022 · 8 comments · Fixed by #6175
Assignees

Comments

@hbharding
Copy link
Contributor

hbharding commented Apr 21, 2022

Summary

There have been several instances when using EuiDescriptionList where the default 16px spacing feels too generous. I've run into this when working on flyouts, expanded table rows, or dashboard-esque panels where we provide summary information. In these cases, i've found that 8px feels more appropriate. This could also work well on smaller devices / breakpoints. I've seen engineers work around this by using multiple EuiDescriptionLists with just a single title/description inside so they can control the spacing with EuiSpacer or custom CSS. This feels a little messy and not very semantic. Ideally all items are rendered inside a single <dl>

Proposal

Add a prop to EuiDescriptionList to enable this. The prop should default to our current 16px spacing so current implementations are unaffected. I imagine this prop would be ignored when using the condensed layout option. I'm open to ideas on what we call this prop. I like spacing, which would accept sizes such as s (8px) and m (16px, default). However, this prop name isn't used anywhere else (AFAIK) and might be opening a can of worms. Open to ideas!

image

Next steps

Curious how others feel about this. If there's consensus, I'd be happy to take this on / assign myself. Shouldn't be too hard and it's been awhile since I've contributed :)

@cchaos
Copy link
Contributor

cchaos commented Apr 21, 2022

I think it's a fair proposal and I would name the prop gap and use the actual CSS gap property.

@hbharding
Copy link
Contributor Author

Ah good call. gap sounds perfect. Thanks for the tip! I'll add this to my back burner for when I have time.

@hbharding hbharding self-assigned this Apr 21, 2022
@cchaos
Copy link
Contributor

cchaos commented Apr 21, 2022

@hbharding We're currently converting our components to CSS-in-JS. This one will likely be on the early side since it's one of the simpler components. Should we consider this issue actually open for grabs so if we get to the conversion before you've tackled this issue, should we go ahead and implement it?

@hbharding
Copy link
Contributor Author

hbharding commented Apr 21, 2022

Good point. Prime candidate for CSS-in-JS conversion. This isn't urgent, so I'll hold off until then and let someone else tackle it. Thanks!

@hbharding hbharding removed their assignment Apr 21, 2022
@snide
Copy link
Contributor

snide commented Apr 21, 2022

While you're in here I would 💯 deep dive on how these things wrap as well. Most of the ugly usages of this component are due to people stuffing way too much in there. In some cases some line zebraing would go a long way to legibility.

@hbharding
Copy link
Contributor Author

Happy to dive in! I can't count the number of times I've witnessed what you're describing @snide. Sometimes our team has opted instead to use a "headerless" 2 column EuiTable which provides control over column width and comes with horizontal dividers and a hover effect on each row... Probably not the most semantic implementation, and all the dividers sometimes feel a bit heavy.

Some other ideas I might consider:

  • For column layouts, an option (or built in behavior) to adjust the column ratio. It's currently 1:1, which isn't always ideal. It can cause excessive word wrapping and a large gap between the title and description when the title is short and the descriptions are long. When the content is known, this option could be useful.
  • Adding an option to EuiDescriptionListDescription so that the value can be copied. This is a pretty common pattern across Kibana that has a lot of different implementations. Sometimes a tooltip appears that says "Copied" and another says "Copied to clipboard". I think I've even seen a toast appear.

@cchaos
Copy link
Contributor

cchaos commented Apr 21, 2022

With regard to bullet one, I just implemented a similar thing for EuiDescribedFormGroup where I added a ratio prop and then simply used the flex-grow property to adjust the scale of each column: https://github.com/elastic/eui/pull/5756/files#diff-ceeb6d2fc8ab3fb3e87ecbb3c5f9be85ee2f2132d2e9192d197acbcbb116b5f5

Your second bullet though, needs more spec. I'm not sure we'd want it to be directly part of this component. We already have the EuiCopy wrapping component that creates those tooltips as you mention.

@hbharding
Copy link
Contributor Author

Your second bullet though, needs more spec. I'm not sure we'd want it to be directly part of this component. We already have the EuiCopy wrapping component that creates those tooltips as you mention.

I was in the middle of editing my comment and you beat me to it haha. I was going to suggest a wrapper component, and forgot EuiCopy exists. Anyway, agree on needing more spec. Just an early thought.

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 a pull request may close this issue.

4 participants