-
Notifications
You must be signed in to change notification settings - Fork 841
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
Comments
I think it's a fair proposal and I would name the prop |
Ah good call. |
@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? |
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! |
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. |
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 Some other ideas I might consider:
|
With regard to bullet one, I just implemented a similar thing for EuiDescribedFormGroup where I added a 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. |
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 multipleEuiDescriptionList
s with just a single title/description inside so they can control the spacing withEuiSpacer
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 thecondensed
layout option. I'm open to ideas on what we call this prop. I likespacing
, which would accept sizes such ass
(8px) andm
(16px, default). However, this prop name isn't used anywhere else (AFAIK) and might be opening a can of worms. Open to ideas!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 :)
The text was updated successfully, but these errors were encountered: