search

conversionxl / aybolit

Lightweight web components library built with LitElement.
https://conversionxl.github.io/aybolit/
MIT License
7 stars 8 forks source link

feat(cxl-ui): implement dashboard card design #293

Closed freudFlintstone closed 1 year ago

freudFlintstone commented 1 year ago

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

Design specs

github-actions[bot] commented 1 year ago

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 commented 1 year ago

Stories: Screenshot from 2023-07-07 15-17-15 Screenshot from 2023-07-07 15-16-23

anoblet commented 1 year ago

I would add dependencies to the PR description:

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

Dependencies:

freudFlintstone commented 1 year ago

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 commented 1 year ago

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

freudFlintstone commented 1 year ago

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 ?

freudFlintstone commented 1 year ago
  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 commented 1 year ago 
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 commented 1 year ago
  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

freudFlintstone commented 1 year ago

@pawelkmpt I"ll check what's going on with the final line, sorry about that.

freudFlintstone commented 1 year ago

@pawelkmpt Some observations for this review:

pawelkmpt commented 1 year ago
  • added stories for all card types

Thank you! Look good πŸ‘

  • background colors are not available from cxl-lumo-styles, so the colors are set in the this component scope only until we assess what tokens might be needed, considering other parts of the redesign

πŸ‘

  • Card heights might need a design decision. For now, I chose to follow the figma specs and set a min-height: 270px

It's good you chose what Figma defined

but the downside is that 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.

I noticed this behavior. Can we fix it? It can't work like that

freudFlintstone commented 1 year ago
  • Card heights might need a design decision. For now, I chose to follow the figma specs and set a min-height: 270px

It's good you chose what Figma defined

but the downside is that 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.

I noticed this behavior. Can we fix it? It can't work like that

So, yes, but not without different cons. @heshfekry, your input would be useful here too. Ideas:

  1. Set the card height and the body content height: This would solve it, all cards would look the same height and have the footer in the same position, and also expand when needed without moving the footer. However, what should we do if the body content (course description) is longer than expected?

  2. Go back to what I saw before this, having only a minimal height. THis means that cards might be taller if the content is longer, but it solves the footer situation. The other glaring downside is that the grid won't look as nice, unless we find a minimal-height that would be enough for most descriptions

    If it helps, I can make both these cases a story

heshfekry commented 1 year ago
  • Card heights might need a design decision. For now, I chose to follow the figma specs and set a min-height: 270px

It's good you chose what Figma defined

but the downside is that 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.

I noticed this behavior. Can we fix it? It can't work like that

So, yes, but not without different cons. @heshfekry, your input would be useful here too. Ideas:

  1. Set the card height and the body content height: This would solve it, all cards would look the same height and have the footer in the same position, and also expand when needed without moving the footer. However, what should we do if the body content (course description) is longer than expected?
  2. Go back to what I saw before this, having only a minimal height. THis means that cards might be taller if the content is longer, but it solves the footer situation. The other glaring downside is that the grid won't look as nice, unless we find a minimal-height that would be enough for most descriptions

If it helps, I can make both these cases a story

  1. Option 1 has the most legs because option 2 destroys the grid. The View course CTA moving up and down is less of a problem. With content longer than normal, lets see an example and assess. Logic solution would be to increase the overall size of the card a little to make space for a specific number of characters, and we use that to cap the content of the description in course production. This should have a limit anyways. What is the character limit now?

  2. I see dodgy spacing and size on bottom left card here.

image

  1. How is the new state applied?
freudFlintstone commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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.

image

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 commented 1 year ago

@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.

freudFlintstone commented 1 year ago

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