Skip to content
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

feat(cxl-ui): cxl-card-grid component, light and course card examples #370

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HenerHoop
Copy link

https://app.clickup.com/t/86ayk4p69

To add a title (for e, wrap it inside the card-grid section and insert the title

To use as a section with a title, wrap cxl-card-grid inside the cxl-section. Example:
<cxl-section> <h3 style="margin: var(--lumo-space-xl) 0">Completed Courses</h3> <cxl-card-grid></cxl-card-grid> </cxl-section>

Copy link

github-actions bot commented Dec 11, 2023

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 69.33 KB (+0.79% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.89 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 28.16 KB (0%)
packages/cxl-ui/pkg/dist-web/vendor.js 136.06 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 246.61 KB (+0.22% 🔺)

@HenerHoop HenerHoop marked this pull request as ready for review December 13, 2023 07:45
@HenerHoop
Copy link
Author

Task linked: CU-86ayk4p69 Completed course section

Copy link

@pawelkmpt pawelkmpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide better description in the commit message and PR + show an example. It's not clear at the first glance why this is needed

Copy link

@freudFlintstone freudFlintstone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, I think we don't have a wrapper that would easily achieve the grid design (assuming you checked), and we can reuse this in the future. But it can use some improvements. See my other comments.

Comment on lines 7 to 10
::slotted(.grid) {
display: grid;
grid-template-columns: repeat(1, 1fr);
gap: var(--lumo-space-m);
Copy link

@freudFlintstone freudFlintstone Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the need to slot in a .grid element? If we style the host itself to be the grid container, we achieve the same result and allow parent elements to adapt the style if needed. That's important in our setup, to be able to easily re-use the element in wp and make changes by applying desired classes. Just slot the cards directly.

Comment on lines 1 to 7
cxl-card-grid {
cxl-light-card,
cxl-course-card {
width: 100%;
max-width: none;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same problem. By styling the component :host directly, we avoid unnecessary complexity with global stylesheets. We should only use global styles when there's no other way.

Copy link

@pawelkmpt pawelkmpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look good on storybook. Big cards cause horizontal scrollbar.

Screenshot 2023-12-14 at 13 37 45
Screenshot 2023-12-14 at 13 37 53
Screenshot 2023-12-14 at 13 38 06

Small have lots of whitespace, even in situation when more cards could fit in one line
Screenshot 2023-12-14 at 13 40 25
Screenshot 2023-12-14 at 13 41 01

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants