-
Notifications
You must be signed in to change notification settings - Fork 83
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
base: main
Are you sure you want to change the base?
feat: Card component #8154
Conversation
dev/card.html
Outdated
There was a problem hiding this comment.
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?
Quality Gate passedIssues Measures |
|
||
/** @protected */ | ||
render() { | ||
return html`<slot></slot>`; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?").
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
- Ditch their implementation for yours
- Customize your implementation because it's not enough
- 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.
Documentation is lacking CSS custom props/styling. Not sure if anything else. Oh, tests, of course 😄