-
Notifications
You must be signed in to change notification settings - Fork 158
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
[Card section carousel] Discuss how to avoid adoptors setting too large number of cards per page #4940
Comments
@RobertaJHahn We have short-term (band-aid) fix for #4918 and #4919 already (#4921) so that January release doesn't have to wait for the resolution of this issue, FYI. |
My thoughts: The maximum number of cards or images sort of depends on how many columns of the grid the adopter is using. Adding some examples of my thinking here... I don't know if this is too complicated to achieve in storybook?
|
Configuration | Number of Cards for sm breakpoint |
Md |
Lg –Max |
---|---|---|---|
Default | 1 | 2 | 4 |
Large | 1 | 1 | 2 |
12
columns used on desktop view
Configuration | Number of Cards for sm breakpoint |
Md |
Lg –Max |
---|---|---|---|
Default | 1 | 2 | 3 |
8
columns used on desktop view
Configuration | Number of Cards for sm breakpoint |
Md |
Lg –Max |
---|---|---|---|
Default | 1 | 1 | 2 |
Large | 1 | 1 | 1 |
Thanks @oliviaflory for your thoughts! Sounds that rather than preventing our adaptor from using too many cards per page, we want to provide good set of number (of cards per page) as (Storybook) examples. Is it correct? |
I think so @asudoh, and based on our explorations above, the maximum number or cards per page doesn't exceed 4 (in desktop) at this point. |
Adds a couple of stories, one takes up 12 Carbon grid cells (of `lg` and beyond breakpoint) at the right, and another takes up 8 Carbon grid cells at the center. Also adds knobs to select alternate number of cards per page, for those stories/breakpoints combinations. Refs carbon-design-system#4940.
### Related Ticket(s) Refs #4940. ### Description Adds a couple of stories, one takes up 12 Carbon grid cells (of `lg` and beyond breakpoint) at the right, and another takes up 8 Carbon grid cells at the center. Also adds knobs to select alternate number of cards per page, for those stories/breakpoints combinations. ### Changelog **New** - Stories for `<dds-carousel>`, one takes up 12 Carbon grid cells (of `lg` and beyond breakpoint) at the right, and another takes up 8 Carbon grid cells at the center. - Knobs to select alternate number of cards per page.
…-system#4974) ### Related Ticket(s) Refs carbon-design-system#4940. ### Description Adds a couple of stories, one takes up 12 Carbon grid cells (of `lg` and beyond breakpoint) at the right, and another takes up 8 Carbon grid cells at the center. Also adds knobs to select alternate number of cards per page, for those stories/breakpoints combinations. ### Changelog **New** - Stories for `<dds-carousel>`, one takes up 12 Carbon grid cells (of `lg` and beyond breakpoint) at the right, and another takes up 8 Carbon grid cells at the center. - Knobs to select alternate number of cards per page.
Per @oliviaflory |
FYI: @asudoh Adding this conversation to the "DDS Team Huddle 2021-03-02" for group discussion |
Discussed this issue during Team Huddle time today. The decisions are to simply remove knobs and remove any storybook entires that suggests the component will be used with different number of cards per page. |
The problem
While
<dds-carousel>
sets the default number of cards per page that best suits the available viewport width,<dds-carousel>
also allows our adaptors to specify arbitrary number of cards per page, viapage-size
attribute. It caused weird UI caused by extremepage-size
, as #4918 and #4919 reports.The solution
This issue is to discuss how to prevent such misconfiguration. A couple of options I can rattle off are:
page-size
attribute value that exceeds that maximum number(Just example of 2., the actual numbers should be determined by design team)
sm
breakpointmd
lg
SMALL
DEFAULT
LARGE
CC @jeffchew @kennylam @oliviaflory
The text was updated successfully, but these errors were encountered: