eui icon indicating copy to clipboard operation
eui copied to clipboard

[EuiDescriptionList] Add option for vertical spacing

Open hbharding opened this issue 3 years ago • 8 comments

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

hbharding avatar Apr 21 '22 18:04 hbharding

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

cchaos avatar Apr 21 '22 18:04 cchaos

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

hbharding avatar Apr 21 '22 18:04 hbharding

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

cchaos avatar Apr 21 '22 18:04 cchaos

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 avatar Apr 21 '22 18:04 hbharding

While you're in here I would :100: 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.

snide avatar Apr 21 '22 19:04 snide

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.

hbharding avatar Apr 21 '22 20:04 hbharding

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.

cchaos avatar Apr 21 '22 20:04 cchaos

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.

hbharding avatar Apr 21 '22 20:04 hbharding