Add column modifier to list object #1399
Conversation
🦋 Changeset detectedLatest commit: 4d7527b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
tylersticka
left a comment
tylersticka
left a comment
There was a problem hiding this comment.
Neat! One question inline
src/objects/list/list.scss
Outdated
| @include fluid.grid-gap( | ||
| breakpoint.$s, | ||
| breakpoint.$xl, | ||
| size.$spacing-gap-fluid-min, | ||
| size.$spacing-gap-fluid-max | ||
| ); |
There was a problem hiding this comment.
Is it intentional that we're setting grid-gap rather than column-gap here?
There was a problem hiding this comment.
No I missed this, thanks!
src/objects/list/list.stories.mdx
Outdated
| columns: { | ||
| control: { | ||
| type: 'range', | ||
| min: 2, |
There was a problem hiding this comment.
Should min be 1? Otherwise there isn't a way from the story controls to enable and then disable columns.
src/objects/list/demo/columns.twig
Outdated
| @@ -0,0 +1,15 @@ | |||
| {% if columns and columns > 1 and columnsBreakpoint and columnsBreakpoint != 'none' %} | |||
There was a problem hiding this comment.
Is it intentional that columnsBreakpoint is required? Is there no use case where someone would want columns on by default?
There was a problem hiding this comment.
How would someone use multiple breakpoints?
o-list--2-columns o-list--3-columns@xl
There was a problem hiding this comment.
@tylersticka Would something like this work better?
{% if columns and columns > 1 %}
{% set _list_class = 'o-list--' ~ columns ~ '-column' %}
{% elseif columns and columns > 1 and columnsBreakpoint %}
{% set _list_class = 'o-list--' ~ columns ~ '-column' ~ columnsBreakpoint %}
{% else %}
{% set _list_class = '' %}
{% endif %}
There was a problem hiding this comment.
That would alleviate some of the problems, but if you wanted to do something like this:
o-list--2-columns@s o-list--3-columns@m
I think it would prevent that?
(This is one of the reasons I've just been using a class property for defining the class attribute in some of my other patterns. It gets complicated pretty quickly!)
There was a problem hiding this comment.
(This is one of the reasons I've just been using a class property for defining the class attribute in some of my other patterns. It gets complicated pretty quickly!)
Good idea! 👍 🙂
There was a problem hiding this comment.
Something like this?
args={{
class: 'o-list--2-column@s o-list--3-column@m'
}}
Overview
This PR adds a modifier so we can divide lists into multiple columns.
Screenshots
Testing