-
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 slots #8233
base: main
Are you sure you want to change the base?
[feat] Card slots #8233
Conversation
I’m still going to try and implement this without using CSS grid gaps, relying on margins on the content, and using named grid areas to place the content. Note, that the implementation would be much easier if Chrome didn't have a bug and it supported the |
I’m now asking, if this complexity is warranted, or should we limit the slots and/or name them differently. A more simplified version would be, for example, to have prefix, title, content, and suffix slots. If you need a subtitle, you add that in the title slot, after your main title. This means that the styling of the content would be left for the developer vs in this first prototype where some content is styled automatically. |
It's not what I expected to be honest.. but your vision looks more like a card in a card sense 🙂 I'm not sure if this creates more problems than it would solve with the amount of slots.. like three slots for the title (including subtitle and badge).. this feels kinda overenginered and hard to configure correctly or adjust to e.g. project needs for example the title has to be above the image or the badge before the title. I would personally go with something like this:
Prefix slot could be the exchange for the title slot for people or could be used to add a close button that with some css flows over the image Did people really want a horizontal card? I've rarely seen some.. but when I see them it's like image left / everything else on the right.. probably something like this:
This would probably need a little bit more DOM structure or some CSS voodoo |
I think this approach is much better. Card is conceptually a "simple" component, but there are still number of choices involved. So this will be opinion based. But that is exactly the point. When you are not a designer, and just want a component where those opinion based design choices has been made for you and you have some API for couple of alternatives. Like here for example media content is naturally optional, and it can stretched if you want. I would aim for a pareto here. I.e. have features and options to cover 70-80% typical scenarios. In the use cases where this is not enough, it is better to write a custom component for the specific scenario. |
I see the first iteration and this prototype at the opposite ends of the “complexity-opinionation-spectrum”. The first iteration is very simple and has very little opinion about what you do with the card. This prototype is complex, and has plenty of opinion regarding the content. Now we’re supposedly looking for the sweet spot somewhere in the middle, in addition to agreeing what are the opinions we want to bake in by default. As an example, which one do you prefer: <vaadin-card>
<div class="font-l text-header semibold leading-xs">Title</div>
<div class="text-secondary leading-xs">Subtitle</div>
</div> <vaadin-card>
<div slot="title">Title</div>
<div slot="subttitle">Subtitle</div>
</div> Or what could some middle ground between them look like? |
Could you consider adding the image in the content slot in this case? |
Here’s where I picked up the horizontal variant use case: https://m3.material.io/components/cards/guidelines
Yeah, I’m arriving at the same conclusion, which relates to
that as well. I started with a “footer” slot, but changed that hastily to suffix when I worked on the horizontal variant, assuming you’d want any possible action buttons at the end of the card. But you likely still want them at the bottom of the content/card, and there’s likely another slot for the suffix content, like you suggested. |
Somewhere along the spectrum is something like this: <vaadin-card>
<img slot="media">
<vaadin-card-header>
<div slot="title">Title</div>
<div slot="subtitle">Subtitle</div>
</vaadin-card-header>
<vaadin-card-content>
...
</vaadin-card-content>
<vaadin-card-footer>
<button>Action</button>
</vaadin-card-footer>
</vaadin-card> |
Definitely the second one with less styling (which of course is the opposite of my arguments from above). I think the thing that is mostly off-putting myself is the suffix slot of the title. Don't get me wrong.. we also use that position a lot.. it probably comes from the fact that the prefix slot is missing compared to other components where (normally) both kinds are available. (Now I'm also making it more complex.. even tho some could argue it's less complex CSS (hopefully)) OT: do you see a card as section or article from an accessibility standpoint? And do you also have opinion on the hX-lvl of the title and subtitle or do you let this completely free for people to do with the slotted content? Which of course also means aria-labelledBy association with the card.
Of course 🙂 people always find solutions.. they just have to understand that your nice fitting/scaling is not available out of the box which they get from your media slot
Now I understand where your vaadin-item thought comes from.. because that first one really looks like an selectable item and not like a vaadin-card to me. But using different components based on available space is probably a little bit overkill.
From a semantics standpoint this looks kinda better.. but I've got the feeling it could confuse people more than it helps or I might be totally wrong because I'm not your typical react / typescript user.. slots might come with the benefit that you could theoretically also do something like this |
I’m not sure. I was thinking of leaving that decision for the developer. Would you expect a default role?
The plan is that you choose what you slot in, the component doesn't have an opinion. You can slot in a heading element, but they you might need to tweak the styles yourself, as Lumo has default styling for heading elements, and the Card component's shadow DOM styles can't override those. If you slot in a heading element, perhaps we should pick that up and connect it with the card container with aria-labelledby automatically? |
That sounds like a Card style variant to me, that re-arranges the slots visually using css grid. (IIRC we do the same in other components, and from an a11y POV it should not be problematic since we're talking about an image).
Although you can just not use slots you don't want to use, I do think we could well have just a single slot between image and content, and an API that allows you to just pass a string but also supports placing elements there if you need a subtitle and/or a badge or something on the side. |
@@ -54,7 +54,7 @@ const card = css` | |||
var(--vaadin-card-box-shadow); | |||
} | |||
|
|||
:host(:where([theme~='cover-media'])) ::slotted([slot='media']) { | |||
:host(:where([theme~='cover-media'])) ::slotted([slot='media']:is(img, video, svg, vaadin-icon)) { |
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 also contain picture?
Personally I would go with section but only if people have supplied either a title text or component so that it can be associated together.. having a section without headline does not help
Definitly! See above 🙂
Is it okay to use? I remember nasty accessibility problems when e.g. using visual ordering vs DOM ordering with tab key and such making problems again.. Good job with the new DOM structure! Looks really promising |
Yes, I agree. I have had a TODO idea in my mind to do it very similar fashion. There are couple of better CSS ideas here than in the model that I had in my head. |
I think it would be a problem, if you place something else than visuals/decoration in the media slot. But if it’s an image/icon, it shouldn't matter, if it’s not announced at all. But yeah, it’s something to point out in documentation if we do something like that (a variant that changes the visual order). |
I recorded a short video going through the current prototype, hoping it’ll give a good overview of the features at the moment, making it easier to give feedback. Apologies for the poor quality/low resolution – GitHub has a 100MB limitation on attachments. card-prototype-demo.mov |
Looks really good! Only thing I would probably ask for: add some CSS for the horizontal + low resolution that the card back to normal.. otherwise it might look really ugly / unfinished. Btw: you might wanna share the video in the forum.. i think I saw multiple people in the past requesting this component.. might be really good to get their feedback as well |
Sorry, I didn't quite understand what you meant.
Yeah, thanks for the suggestion. I’ll do that. |
:host(:is(:has(> :not([slot])), [has-content])) { | ||
--_content: 1; | ||
} |
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.
Looks like this doesn't support plain text as card content, e.g. the following works:
<vaadin-card>
<div>Content</div>
</vaadin-card>
But the following does not work (the content isn't visible):
<vaadin-card>Content</vaadin-card>
); | ||
this.toggleAttribute('has-header-prefix', this.querySelector(':scope > [slot="header-prefix"]')); | ||
this.toggleAttribute('has-header-suffix', this.querySelector(':scope > [slot="header-suffix"]')); | ||
this.toggleAttribute('has-content', this.querySelector(':scope > :not([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 could be probably updated to check for the actual slotted nodes as querySelector()
doesn't return text nodes from the default slot. I could change the logic to use SlotObserver
helper that we have since it provides the access to the list of current nodes using slot.assignedNodes({ flatten: true })
.
|
||
/** @private */ | ||
_onSlotChange() { | ||
// Chrome doesn't support `:host(:has())`, so we'll recreate that with custom attributes |
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.
I wonder if we want to provide has-
attributes for all browsers? This approach is widely used by existing components e.g. has-error-message
etc and IMO it would make card styling easier.
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.
Did some tweaks to visual tests to improve readability in #8369 and merged it to this PR.
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.
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.
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.
Setting fixed height on the vaadin-card
with overflowing content causes it to break:
While setting overflow: auto
helps with content overflow, the footer would still end up behind the content.
I think we should fix this as users might want to restrict card height e.g. to have multiple cards of same height or max-height
.
Quality Gate passedIssues Measures |
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.
The rounded right side corners of the image look kinda off in this material version, or?
The same goes for another image below.. or is this intended in material design? The lumo versions looks way better in this regard 😅
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.
Good catch, thanks. Yes, we need to fix the Material version.
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.
Tested in latest Chrome and it seems to look correctly, maybe it's related to outdated Chrome version used by Material tests as it differs from Lumo setup (some CSS properties could be not supported yet in Chrome 88).
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.
UPD: my assumption above was correct. Created #8383 to update Chrome version.
.
Yeah, that’s a bit unfortunate. The value for grid-template-rows comes from But for some reason Chrome and Safari don't treat I know it feels a bit "icky" to rely on this behavior (using an invalid value to end with the initial value for a property), but it (almost) does the trick. I’d say we don't care about this edge case right now. But we should be able to fix this with something like |
This is the same issue as the one I described above. |
This prototype introduces six slots to the Card component: media, title, subtitle, title-suffix, content, and suffix. The layout can be vertical (the default) or horizontal.
The media slot is meant for images, icons, or avatars, but you can put anything there if you want to. The media component/element layout can be controlled slightly with the
cover-media
andstretch-media
theme variants, which make it cover or stretch the full width/height of the card.The implementation has some quirks/limitations, especially the horizontal variant:
:not([slot])
), but no title or subtitle, the content element will be below the media, instead of next to it. Similarly with the suffix element.