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: Card component #8154

Merged
merged 21 commits into from
Nov 19, 2024
Merged

feat: Card component #8154

merged 21 commits into from
Nov 19, 2024

Conversation

jouni
Copy link
Member

@jouni jouni commented Nov 14, 2024

Documentation is lacking CSS custom props/styling. Not sure if anything else. Oh, tests, of course 😄

Light Dark
localhost_8000_dev_card html (4) localhost_8000_dev_card html (5)

dev/card.html Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Should I spend time reducing the duplication?


/** @protected */
render() {
return html`<slot></slot>`;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks kinda basic 🙂 no support for header, content and footer like dialog? Unify API and usage for developers before they have to do it again and again

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback! Yeah, we are very much aware of this. We are just not yet sure how we want to implement all of that and what kind of API would be suitable, and want to move quite slowly in the beginning before locking in on anything more specific.

We just spent some hours talking with @rolfsmeds about this, and how card relates to the proposed "item" component, and the other "header-content-footer" component we’re thinking about, how all those fit together ("Are they the same component or smaller pieces you need to compose together to get what you need? If they’re not the same component, how do we share the styles and layouts between them? How do we name all of them? Could they just be utility classes? How does all of that affect discoverability? What is even knowledge?").

Copy link
Member Author

@jouni jouni Nov 15, 2024

Choose a reason for hiding this comment

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

And as you mentioned Dialog, we also touched on that, and will likely want to refactor it to use the "header-content-footer" layout we’re pondering (eventually, whenever that might happen).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for publishing your thoughts 🙂 I was sadly kinda expecting it.. especially combined with the recent discussion you linked.

My concern comes from the way of using / adapting to a new component: what is the benefit for me as developer? If it's just a "wrapper" with no real functionally (header footer and so on) it "feels" unfinished - which lefts me with two options: don't use it.. or use it, extend it and customize it. Often I'm seeing myself doing the second option because - if available - I want to use the official component as base. Which in turn creates problems when "unfinished" components gets later enriched because this can clash with my already enhanced version of it. That's why I mentioned Dialog where exactly that happened when I migrated to your provided header and footer slots. IMHO those "basic" constructs should have been available at the beginning... Therefore I would request them here as well.. even tho it could push this component one minor release in the future when some designs are needed.. it would highly benefit customer before they have to work three times implementing / using it.

  1. Ditch their implementation for yours
  2. Customize your implementation because it's not enough
  3. After your implementation is enhanced.. get rid of customizing

While I agree that it your be beneficial to have a shared item base for dialog, card and what not for.. it feels kinda overenginered IMHO.. yes they could share styles.. but I personally would be against it. This could easily create problems where you wanna style "vaadin-item" however since vaadin-dialog is also using it.. those customizing might leak or developers are not aware of the consequences.. especially when you squeeze in css variables.

packages/card/package.json Outdated Show resolved Hide resolved
packages/card/package.json Outdated Show resolved Hide resolved
packages/card/src/vaadin-card.d.ts Outdated Show resolved Hide resolved
* <vaadin-card> is a visual content container.
*
* ```html
* <vaadin-card class="flex flex-col overflow-hidden">
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to keep this example minimal and not use any CSS classes here since users who are unfamiliar with Lumo utility classes might think they are somehow part of the component. Not a blocker though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, sounds fair. I’ll try to think of an example that doesn't need utility classes on the card itself.

Copy link
Member

Choose a reason for hiding this comment

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

Changed to minimal example for now, we can update JSDoc in a separate PR later.

packages/card/test/card.common.ts Outdated Show resolved Hide resolved
Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

Rebased the PR, updated version in package.json and feature flag to cardComponent.

packages/card/LICENSE Outdated Show resolved Hide resolved
@jouni jouni marked this pull request as ready for review November 18, 2024 09:45
Copy link

sonarcloud bot commented Nov 19, 2024

@web-padawan web-padawan merged commit cfab464 into main Nov 19, 2024
9 checks passed
@web-padawan web-padawan deleted the feat/card branch November 19, 2024 08:58
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.6.0.alpha4 and is also targeting the upcoming stable 24.6.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: December 2024 (24.6)
Development

Successfully merging this pull request may close these issues.

4 participants