LayoutGrid - Add responsive column layout feature (HDS-5692)
#3438
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
didoo
left a comment
didoo
left a comment
There was a problem hiding this comment.
I did a first pass and added a few comments (mostly in sync with others). Once some of the open questions have been answered, I want to give it another pass, there are a few things that may be possible to simplify (but I am not sure, I need time to test)
| '--hds-layout-grid-column-width-md'?: string; | ||
| '--hds-layout-grid-column-width-lg'?: string; | ||
|
|
||
| '--hds-layout-grid-column-fill-type-sm'?: string; |
There was a problem hiding this comment.
[question] since there is already a "default" auto-fill value for --hds-layout-grid-column-fill-type in CSS, why do we need these extra values, that are assigned auto-fill anyway in JavaScript below?
There was a problem hiding this comment.
I refactored how the grid column-fill-types work and are set. I think how I've changed it now makes more sense (in addition to reducing repetitious code) but there may be further ways to optimize.
There was a problem hiding this comment.
I think part of the previous issue is the way I was originally setting it was kind of backwards to what I now think is the better approach I pushed up. Going to see if I can optimize a bit further next though.
Co-authored-by: Jory Tindall <jory.tindall@hashicorp.com> Co-authored-by: Cristiano Rastelli <cristiano.rastelli@hashicorp.com>
KristinLBradley
force-pushed
the
kristin/hds-5692-grid-responsive
branch
from
d77732e to
ddea1dd
Compare
…option to pass in array
…exity, update Showcase with additional examples
didoo
left a comment
didoo
left a comment
There was a problem hiding this comment.
Left a few comments, but I think we're in the right direction.
| if (this.args.columnMinWidth) { | ||
| inlineStyles['--hds-layout-grid-column-min-width'] = | ||
| this.args.columnMinWidth; | ||
| inlineStyles['--hds-layout-grid-column-fill-type'] = 'auto-fit'; |
There was a problem hiding this comment.
[question] usually we use CSS variables in JS to pass down to the CSS dynamic values set by the consumers; in this case, we already know the two possible values (auto-fit and auto-fill) and the conditions for them, so could we simply use CSS classes assigned in the get classNames() getter below, using this same logic?
| export const GAPS: HdsLayoutGridGaps[] = Object.values(HdsLayoutGridGapValues); | ||
|
|
||
| type ResponsiveColumnWidths = { | ||
| sm?: string; |
There was a problem hiding this comment.
[issue?] should this be required, with the current mobile-first approach? otherwise, with this code
<CodeFragmentWithPlaceholderItems
@columnCount={{5}}
@columnWidth={{hash lg="33.33%" xl="25%" xxl="20%"}}
/>
this happens:
alternatively we could use somehow the width/min-width arguments, but I suspect it will make the API more complex
see what you find out, what works better
|
|
||
| // "sm" view (mobile devices) | ||
| .hds-layout-grid--column-width-sm { | ||
| grid-template-columns: repeat( |
There was a problem hiding this comment.
see comment above about the fact that, since this is mobile-first, if the sm is not set then there is no intrinsic width for the columns at this size
There was a problem hiding this comment.
I'll experiment with this more. This is part of why I had the concept of a kind of "default" size in the original implementation.
|
|
||
| <ShwDivider @level={{2}} /> | ||
|
|
||
| <ShwTextH4>With responsive column widths</ShwTextH4> |
There was a problem hiding this comment.
do you think we should have these in a "frameless" page so we can test them? the downside would be that we can't take Percy snapshots of them so they would not be covered for visual regression testing
alternatively, we could create a shared code snippet, to be used here but also in a frameless page, and we could put a link to it like I'm doing here: https://hds-showcase-git-project-solar-phase-1-main-fe-1ffc6c-hashicorp.vercel.app/foundations/theming#demo (see the link under the "demo" section)
5 participants
WIP: Need to confirm screen sizes/breakpoints, etc. we want to support (with designer input)
📌 Summary
If merged, this PR adds responsive column width options to the existing
@columnWidtharg of theLayoutGridcomponent.Preview: https://hds-showcase-git-kristin-hds-5692-grid-responsive-hashicorp.vercel.app/layouts/grid#responsive-column-width
🛠️ Detailed description
@columnWidthoption to allow responsive column widths to optionally be passed in as an object.Views supported:
Usage examples:
columnWidth(3 columns in all views):<Hds::Layout::Grid @columnWidth="33.33%" />columnWidthoverridden in "small view" only:<Hds::Layout::Grid @columnWidth={{hash sm="100%" md="33.33%"}} />columnWidthfor each supported view:<Hds::Layout::Grid @columnWidth={{hash sm="100%" md="50%" lg="33.33%" xl="25%" xxl="20%"}} /><Hds::Layout::Grid @columnWidth={{hash sm="50%" xxl="25%"}} />📸 Screenshots
Extra, extra large (xxl) view:
Extra large (xl) view:
Large (lg) view:
Medium (md) view:
Small (sm) view:
🔗 External links
👀 Component checklist
💬 Please consider using conventional comments when reviewing this PR.
📋 PCI review checklist
Examples of changes to controls include access controls, encryption, logging, etc.
Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.