feat(cxl-ui): implement dashboard card design

[freudFlintstone]

Task: https://app.clickup.com/t/861n0y1vn

Design specs

[github-actions]

size-limit report 📦

Path	Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js	38.3 KB (+6.15% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js	11.87 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js	26.26 KB (+3.85% 🔺)
packages/cxl-ui/pkg/dist-web/vendor.js	133.84 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js, packages/cxl-ui/pkg/dist-web/manifest.js, packages/cxl-ui/pkg/dist-web/unresolved.js, packages/cxl-ui/pkg/dist-web/vendor.js	211.42 KB (+1.54% 🔺)

[freudFlintstone]

Stories:

[anoblet]

I would add dependencies to the PR description:

Clickup: https://app.clickup.com/t/861n0y1vn
Figma: Design specs

Dependencies:

• feat: add dashboard lightweight card #289

[freudFlintstone]

I would add dependencies to the PR description:

Clickup: https://app.clickup.com/t/861n0y1vn Figma: Design specs

Dependencies:

* [feat: add dashboard lightweight card #289](https://github.com/conversionxl/aybolit/pull/289)

Oh, but I did not build on top of that branch. I actually discussed that with @heshfekry, that we would probably have some overlap, but @kertuilves was too far along with her work then, I think, although not ready for me to build on it. Should I wait for her merge and the rebase my branch?

[anoblet]

My mistake. Are we able to add a story for cxl-card[theme="cxl-dashboard"]?

[freudFlintstone]

My mistake. Are we able to add a story for cxl-card[theme="cxl-dashboard"].?

No, it was my mistake, I was actually in the process of changing all of it to theme=cxl-course, or theme=cxl-course-card. I found it confusing to describe the two stories without separating the container's dashboard layout from the card course theme. Your thoughts, @pawelkmpt @anoblet ?

[pawelkmpt]

1. Please clean up git. You need to rebase on top of feat/dashboard.

2. Card has "Read more" part. What happens if there's no "more" to show?

[freudFlintstone]

1. Please clean up git. You need to rebase on top of feat/dashboard.

@pawelkmpt I tried that, but it looks like feat/dashboard is actually a little behind master, on which my branch was based. Can I rebase feat/dashboard from master myself?

[freudFlintstone]

2. Card has "Read more" part. What happens if there's no "more" to show?

Not specified in the design. Should I just hide or disable it? What do you think, @heshfekry ?

[pawelkmpt]

1. Please clean up git. You need to rebase on top of feat/dashboard.

@pawelkmpt I tried that, but it looks like feat/dashboard is actually a little behind master, on which my branch was based. Can I rebase feat/dashboard from master myself?

You can

[pawelkmpt]

Looking good 👍

1. Storybook: we need preview for all kinds of cards (video, playbook, etc.)
2. Does your editor respect .editorconfig rules? We have insert_final_newline = true there but you keep pushing files without final newline.

[pawelkmpt]

Adjust commit message

- feat(cxl-ui): implement dashboard card design
+ feat(cxl-ui): add `cxl-course-card` component

[freudFlintstone]

I'll work on option 1 then, and also make sure those tags don's have that odd spacing when there are many of them.

3. How is the new state applied?

just a boolean attribute on the card:

<cxl-course-card new>
 <content>
</cxl-course-card>

[freudFlintstone]

@heshfekry @pawelkmpt After a lot of testing, I feel like 300px of height is a good value. It will handle 4 lines of description easily. Even if tags and title wrap, it's doesn't stretch that much. I added another dashboard story to show some extreme examples.

[pawelkmpt]

@heshfekry @pawelkmpt After a lot of testing, I feel like 300px of height is a good value. It will handle 4 lines of description easily. Even if tags and title wrap, it's doesn't stretch that much. I added another dashboard story to show some extreme examples.

@freudFlintstone it keeps jumping. I experimented with min-height for <p slot="content"></p> element in and min-height: 110px; solves the problem. Is there anything which stops us from using such value?

Also - what I already mentioned at least once: more slot needs smaller font. The same size as other parts of the card

[freudFlintstone]

@heshfekry @pawelkmpt After a lot of testing, I feel like 300px of height is a good value. It will handle 4 lines of description easily. Even if tags and title wrap, it's doesn't stretch that much. I added another dashboard story to show some extreme examples.

@freudFlintstone it keeps jumping. I experimented with min-height for <p slot="content"></p> element in and min-height: 110px; solves the problem. Is there anything which stops us from using such value?

No, I just haven't tried using that because it's a big difference from the design spec and short descriptions will have a lot of white-space below. But if having a square grid is important, then setting exact heights is a requirement. Also, itś not a fix for everything. As you can see below, we need a fixed height for the title too, which risks an even larger title being cut-off. I'll do some testing on that.

Also - what I already mentioned at least once: more slot needs smaller font. The same size as other parts of the card

Fixing it now. It's being overridden by vaadin-details

[freudFlintstone]

@pawelkmpt I'm re-requesting the review, but we still need to make a decision about whether to enforce some layout constraints or just let it go for now.

[pawelkmpt]

https://www.loom.com/share/2e01dc825c5c4a76a4a9e28461e2817a?sid=f41c2c43-125a-4468-b3b8-76658ffd14f0

@pawelkmpt I'm re-requesting the review, but we still need to [make a decision](https://cxlworld.slack.com/archives/C01HXUNGEPM/p1689686050275289) about whether to enforce some layout constraints or just let it go for now.

TLDR; min-height constraints made cards look worse the before. Let’s revert.

Main thing I didn’t like about cards behavior was:

opening the "read more" section will use flexible space above it, if description is short, which then makes the read more + CTA button move up and down when toggling that section.

As I understand, in order to make “read more” part expand an the bottom and make card height bigger without “up and down jump” effect is setting min-heigh for all card elements (title, content, …)?

If so, we should just accept how it works for now and avoid spending days on it.

Title:

no min height -> too much whitespace below for short titles;
no line limits -> user needs to know the full name of the course;

Title tags: just one line like on the design.

Content tags:

do not limit them to one line. They should wrap with small line-height like shown below;
line clamping -> as mentioned in Loom video - we can’t have multiple open/closed states: on hover and on click. It’s too much for such simple component. Remove it.

Important: before overwriting code, make a backup just in case we'd like to revisit these ideas again.

[freudFlintstone]

No problem, I'll revert all changes manually.

Important: before overwriting code, make a backup just in case we'd like to revisit these ideas again.

I'd have to disagree on this one. The right way to do it is to leave the real commits, so we can revert or cherry-pick using the proper git tools if needed. We can always squash before merging. My apologies if that's already what I was supposed to do, but I understood I should always squash, even during PR process.

PS.: Sorry for the confusing notifications, I accidentally edited your comment instead of quote replying

[pawelkmpt]

@freudFlintstone I noticed component looks bad on small mobile 320px but even 360. Everything should look good in 320px. Please correct it.

Tags are cut in the bottom

[pawelkmpt]

It's almost perfect. I was about to merge and then noticed bottom whitespace below CTAs is bigger then before... Please fix it.