-
Notifications
You must be signed in to change notification settings - Fork 72
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
[BETA] Card grid and checkboxes #748
Conversation
to have a suitable location for playground sub-components when testing complex scenarios
and integrate with KCard
Helps to preserve auto-generated props documentation format as intended in the source docstring. Particularly useful when documenting larger objects taking more lines in a prop docstrings.
Some work remains, especially when there are no thumbnails.
in the horizontal layout with small thumbnail. Fixes the thumbnail overflowing and aligns this layout more closely to the designs.
To be able to complete this in a11y way, KCheckbox will need to be updated to allow for more flexible label. Due to how KCheckbox is implemented it seems that currently there is no way how to set the card title as its checkbox label. There may be some more a11y requirements needed, to be discussed.
My first reaction to the spec is that adding the KCardGridItem seems to add a lot of boilerplate to using KCardGrid - if a KCard is inside a KCardGrid it should just act accordingly, requiring it to be wrapped in an additional component that doesn't have an API of its own seems like extra boilerplate for a developer. My second is that having the low level API in pixels is useful sometimes, but that it might be useful to have a higher level API, similar to what we have done in Kolibri where we specify the dimensions of the grid in cards, rather than pixels: https://github.com/learningequality/kolibri/blob/2faa5ea3f0abb5bbaa180d184935a5e9e462d44b/kolibri/plugins/learn/assets/src/views/ExploreLibrariesPage/index.vue#L134 Having some utilities like this as a composable that works with the grid will be important to ensure that we show the correct number of cards for a specific layout (without having a 3 in one row, then 1 on the next overhang, for example). |
@rtibbles Thanks for looking at it, Richard.
👍 The primary reason for
I don't quite understand yet - what pixels do you mean here exactly? Card's width is not specified in pixels, we just tell Row's height and row/column gaps are specified in pixels because that seems to be a pretty clear design requirement (designs).
I think I will need to understand this better too because in the new designs for grids we should always show a specific number of columns on a given breakpoint, and 'one in the next overhang' is commonly present there - so the emphasis is on consistency of columns number on certain breakpoints. Regarding integration |
@rtibbles I'd like to have those questions clarified, but wanted to mention that generally I can see a point having a composable as an alternative approach to the I think there'd be a bit more of internal complexity around it that would need to be put into place for situations when there's more |
So I found some time to think through the feedback and conversations in calls, and I think in the next step would be:
@rtibbles I don't yet have the exact idea about the Thanks all, hopefully the second version will turn out nicely thanks to the above :) |
@AllanOXDi it's not ready for review yet - there will be lots of code changes. I will make sure to invite all people involved in cards as soon as it's time to move it out of the draft stage. |
The width of the grid was the main thing I was concerned with here (and maybe I misread the spec) - but I recall us having a lot of issues with having variable width cards within a breakpoint (min-width and max-width behaviours defined) but then a fixed width grid as a whole. So, I guess the main takeaway here is that if we are defining the size of the cards, we should only define the number of columns we want, and not the exact width of the grid.
If that's the case, that's a big flaw in the design - it causes a huge amount of wasted visual space, especially on smaller screens where it is most needed. Nothing I am proposing involves any changes to consistency in number of columns, just an emphasis on showing full rows where ever possible, to avoid wasting screen real estate. |
@rtibbles thanks a lot, I think I can understand better what you were trying to say.
Ah okay, I think that makes sense and should be satisfied. The mechanism now is that you put in the number of columns,
Ah so basically you mean that when we have two columns, that is two cards are displayed in one row, then those cards should occupy that space fully, right (versus having fixed max height and leaving blank space in the other half of the row)? I think that's what is suggested on designs. I interpreted your first note differently and though you might be suggesting we change number of columns in such a case. |
@rtibbles Technically the cards don't have any width settings on themselves, they always take all the available width that is given to them by the grid. We can still control their min width and max width, but that would rather be by limiting the space available for grid and configuring the number of columns. I will wait for the confirmation that this behavior is aligned well with your thinking about overcoming the trouble you mentioned. This discussion makes me think I should mention this in the doc, because typically the first thought would be to set min and max height directly on cards, which we're trying to prevent. |
@rtibbles that said, this can still happen, but in the new designs the cards are bigger overall and space seems reasonably distributed to me: If you see some improvements to do here, we can discuss with designers. Technically, I think I have a good idea about possible behaviors and can adjust the way we configure it to be flexible enough to allow to tweak these things :) |
I think the key takeaway @rtibbles was suggesting is that we work to have a number of cards by default that corresponds to the number of columns. There was quite a lot of effort over the last year to not have "remainder" if not needed, and to have the default number of items == the number of columns on each view, with it changing dynamically. For example, in Kolibri currently we show 3 items by default on a 3-col row. but the same display, on a tablet screen, would have 4 items displayed, in 2 rows, 2 columns. The only time the overhanging item should appear is if it is the last item of the total (i.e. you get to the end of "show more" with a single item remaining to go in the row). Or, if you are looking at a folder that has only one item in it, etc. But the default display should otherwise be matching the number of items to the number of columns. I am not totally clear whether this is the responsibility of the grid managing that dynamically, or if it really has to do with the places that we make use of it. It seems potentially more useful to have the grid manage this itself, but also possibly more complicated. We can discuss more and I think it would be fine to do this in follow up if needed. As long as we make sure we're maintaining that behavior when we're using the new grid for the |
I think it's how it works already, but I am not sure if I haven't missed something else. I didn't really consider this explicitly but rather worked from thinking through the designs linked and all sorts of things they imply. Would you have a look at "Default '1-2-3' grid, vertical cards, small thumbnail" and "Layout configuration overriden via props" sections in the playground https://deploy-preview-748--kolibri-design-system.netlify.app/playground/ and confirmed this is what you mean? There you can see the number of columns changing dynamically. For each breakpoint, we are able to specify the number of columns/cards. Is this what we need or is there more to it? In |
@marcellamaki You can also see code for the playground here https://github.com/learningequality/kolibri-design-system/pull/748/files#diff-8d9f8ad2e4bcdbda8e50460f2774ee0b97a52274cd1870a9842e52dba7f6676f if it helps to evaluate if this approach resolves what you're trying to say. I think in the code that @rtibbles referenced earlier, the computation was about determining |
From the designs, this differs place to place, but there are only three repeating patterns. So the grid manages it for most common patterns but is still configurable to allow for override. After you've previewed the playground page and code, let me know if this suffices the scenarios you were thinking of. Further improvement could be to not base this on the window breakpoint but operate based on the space available in a given element - but yeah this would be better to discuss further and have designs for if it that's something we'd want :) |
@marcellamaki Best to play around with the provided live examples first, and then no problem to chat on a call if it seems we're still not quite aligned :) I think the confusion is clearer now since we were talking from different assumptions about grids, I believe. Let me know what works best. Thanks for offering the follow-up, I'd rather to take care of this sooner than leaving for later since these functionalities, together with a11y, is the main point behind having |
@@ -16,7 +16,6 @@ code { | |||
padding-left: 0.25em; | |||
font-family: 'Source Code Pro', monospace; | |||
font-weight: 400; | |||
white-space: nowrap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be imagining this, but I feel as though I've seen this change in a few PRs lately. I am not sure if/how you would know this, but.... does that seem familiar to you? do you know what's going on with this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's details in the commit message :) f0a1dad
Visually, this change allows preserving nice format for the layoutConfig
example here
I don't know why it was there originally though, but haven't noticed any issues yet after removing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I recall, it would show on one line otherwise, which is not readable (we can still achieve one line format by formatting the source docstring accordingly - so this is about more precise transfer of the source string to auto-generated docs)
Leaving a note here that with @marcellamaki we confirmed that the grid behavior regarding columns overall satisfies what was being discussed above. Latest update is that I already transitioned lots of work on base layouts to production version and have another PR to |
I experimented with implementing Experiment no.1
would result in switching layout on the fly, something I want to definitely as it has negative impact on user experience. So here we call it sooner:
But we'd need to add another information about what's In such approaches we'd also need to add I quite like this API and so far was finding ways to work around these things, but taking into account (1) increasing complexity of public API and struggle with internal implementation, (2) the need for overrides being presumably rare (we don't yet even have use-cases at this point), and (3) I didn't even get to skeletons that will need to have information sooner than as soon as possible :) I lean towards not using Composition API with the purpose of initial configuration and just go with Sounds good or any other thoughts? |
Hi @MisRob! Hmm the first thought I had when I readed the composables idea was to have this as a composable that will be used internally, inside I agree that making this composable part of the public interface of So in my opinion, I think we could still use composables for some of these calculations like the makeComputedCardCount richard suggested, but I think it would be better to use these internally. |
Yes, the internal composable is there and will stay. This is a question of public API exploration, rather. I should have also mentioned that there is whole array of better lifecycle hooks for this purpose, using mounted and setup was to demonstrate opposite poles. That said, execution in setup is fastest. Side note, 'makeComputedCardCount' won't be needed as card grid handles this already by means of its design, as far as I can say from conversation so far. However if we needed different ways to configure this, we could still hook it to 'layoutOverride'. |
Reference for the simpler approach I just hacked quickly 61e95ea |
Closed in favor of #785 |
Description
Introduces
KCardGrid
andKCardGridItem
that enable grid and checkboxes functionality forKCard
. Public APIs can be subject to change - besides providing functional prototype in support of 0.18, this PR serves as a proposal to collect feedback.What works
To be done
In this PR as well as some upcoming PRs.
BaseCard
since the card itself can't beli
anymore, therefore its primary task of enforcing the markup structure is gone. Taking into account thatBaseCard
made some styling updates complex in the past and now adds even more hassle in several places with the addition of checkboxes, it would make sense to discuss its possible merging withKCard
. We could use other means of ensuring the correct structure, such as warnings, documentation guidance, and building larger test suite. This should also help with maintainability and most importantly, I believe it would remove the need forKCardGridItem
that was added because of the checkboxes. So public API would be simpler<KCardGrid><KCard /></KCardGrid>
or with checkboxes<KCardGrid><KCard><template #checkbox><KCheckbox></template></KCardGrid>
.KCard
not rendered withingKCardGrid
References
Figma cards
Figma card grids
Figma loading skeletons
Closes KCardGrid MVP and specification #697
Closes KCard: Allow the height of sections to be configured and related improvements #703
Usage
KCardGrid
props documentationBase grid layouts
KCardGrid
offers two base layouts,'1-2'
and'1-2-3'
, where'1-2'
is the default. These are currently the only multi-column layouts needed.Besides the number of columns explained below, they set column and row gap to
30px
, and ensure the default row height behavior when it automatically adjusts to cards' content (do not miss the "Note on row height" below)'1-2' layout
renders a grid with 1 column on smaller screens, and 2 columns on medium and larger screens. Specifically
'1-2-3' layout
renders a grid with 1 column on smaller screens, 2 columns on medium screens, and 3 columns on larger screens.
Specifically
Note on row height
KCardGrid
provides sufficiently reasonable default behavior for allKCard
s even when the row height is not specified. However, to make some of the cards sections' heights follow the designs as closely as possible,KCardGrid
's consumers are expected to set the row height explicitly so that calculations can be made more accurately. One example would be the height of the thumbnail area in vertical layouts that can't be precisely calculated unless the row height is an actual value (auto
,fit-content
, and similar doesn't count) The row height is typically specified in designs, or it can be estimated after providing the card with all information. See "Layout customization" on how to achieve that.Fixed section heights
KCard
's sections will often need to have fixed heights to allow for consistent horizontal alignment of sections on a grid row. This can be achieved by regular CSS styling applied onKCard
's slots:It is a consumer's task to ensure that slots content can fit the chosen height, for example by text truncation.
Relatedly, if it's expected that some cards will have no content in a section that has content on another cards in the same grid, it is possible to preserve the space occupied by a corresponding slot via
KCard
'spreserve...
props.List of cards with checkboxes
This is a common case of more advanced grid configurations that are demonstrated below. For now, just know that you can use
columns
to override the number of columns of the base grid layouts on their all breakpoints levels.Layout customization
We currently do not have other use-cases than above. However some configuration, such as row height, need to be set either for all breakpoints levels, or on a per breakpoint level basis (there is at least one design that requires nuanced level configuration).
Via shortcut props
The following props override the base layout on all breakpoint levels:
columns
,rowHeight
,rowGap
,columnGap
.For example
uses all configurations of the base
'1-2'
layout, however row and column gap is overriden from30px
to50px
, and row height is set to450px
on all breakpoint levels.These props are the same as level attributes of the
layoutConfig
that offer a simple way to override configuration on all breakpoints.Via
layoutConfig
propFinally, it is possible to override chosen values of the base grid layouts on chosen breakpoint levels. This can be used to override only some parts of the base layout or to define a completely new layout.
The grid component is built to be easily configurable internally with the same layout configuration object. As an additional benefit, all these configurations can be exposed publicly. If we notice some patterns emerging in consuming apps over time, they can be simply added as a new base layout.
For example:
where
layoutConfig
isoverrides the number of columns on the breakpoint levels 0 and 1, the column and row gaps on the breakpoint level 1, and the row height on the breakpoint level 7.
Available levels:
'level-0'
,'level-1'
,...,'level-7'
.Available level configuration:
columns
,columnGap
,rowGap
,rowHeight
.