Add responsiveColumn option to type of EuiDescriptionList#2166
Add responsiveColumn option to type of EuiDescriptionList#2166cchaos merged 5 commits intoelastic:masterfrom
responsiveColumn option to type of EuiDescriptionList#2166Conversation
|
Before I look to deep at this. Any reason why we wouldn't just do this by default for |
|
I dont think that's necessarily true that columns should by default switch to rows. I think a very common scenario of usage is that the content is small enough that (couple words in each column) they display fine as they are on small screens. These don't tend to be big block level layouts like EuiFlexGroup. It's just text. |
thompsongl
left a comment
There was a problem hiding this comment.
A couple doc suggestions. I'd be good with the code changes provided we keep with the current concept (i.e., not use Dave's suggestions).
src-docs/src/views/description_list/description_list_example.js
Outdated
Show resolved
Hide resolved
src-docs/src/views/description_list/description_list_example.js
Outdated
Show resolved
Hide resolved
src-docs/src/views/description_list/description_list_example.js
Outdated
Show resolved
Hide resolved
snide
left a comment
There was a problem hiding this comment.
Maybe better idea? I'm mostly trying to avoid this.
| /** | ||
| * Turns a column layout into normal row layout on small screens | ||
| */ | ||
| responsive?: boolean; |
There was a problem hiding this comment.
That's a good point. How about we make a new type rather than adding the boolean then?type="responsiveColumn"
Sorry. I just really like to avoid conditional acting props because it's hard as a consumer to know how they actually change things without digging deep into the docs. Right now responsive really only changes this component if it happens to be type="column". It does nothing on row or inline. So to me, this is just a different type, not an additive property. Downside of course is if you end up adding a new type down the line where you always want to have a similar split nature, you're then adding something again, but we haven't really touched this one much over the years.
If responsive={true} by default i'd maybe think different, but if you have to go and hand touch these to turn it on anyway?
There was a problem hiding this comment.
It was more that I was trying to align the prop name/type with how we use it elsewhere. I agree that it does nothing to other display types and can create those dead selectors as you say. I don't really feel strongly either way, but I can change it to your suggestion and see how the component continues to develop (if at all). It could also a be a good test case for this way of adding responsive options.
There was a problem hiding this comment.
You can make the call because I can see your side of it too. If you do go with the responsive prop I'd at least make the check to only apply that class when both values match. Then at least the dom output is a little cleaner.
thompsongl
left a comment
There was a problem hiding this comment.
👍 for creating a new type. Tested locally. Changes LGTM
responsive prop to EuiDescriptionListresponsiveColumn option to type prop of EuiDescriptionList
responsiveColumn option to type prop of EuiDescriptionListresponsiveColumn option to type prop of EuiDescriptionList
responsiveColumn option to type prop of EuiDescriptionListresponsiveColumn option to type of EuiDescriptionList
This was brought up in Kibana where the content dictated needing to switch from column to row layout on small screens.
This does that automatically so that consumers don't have to write the logic for it.
Checklist
[ ] This was checked in dark mode[ ] This was checked for breaking changes and labeled appropriately[ ] This was checked against keyboard-only and screenreader scenarios[ ] This required updates to Framer X components