New component: Initial implementation of `KCard` component

[AllanOXDi]

Description

This PR introduces the KCard component to KDS, It will act a foundational building block for creating various card types within our product eco system. By serving as a base component for creating different card types like lesson cards, resource cards, and channel cards.

Issue addressed

https://github.com/learningequality/kolibri-design-system/issues/530 https://github.com/learningequality/kolibri-design-system/issues/528 https://github.com/learningequality/kolibri-design-system/issues/529

Changelog

• [#PR no]
  • Description: Summary of change(s)
  • Products impact: Choose from - none (for internal updates) / bugfix / new API / updated API / removed API. If it's 'none', use "-" for all items below to indicate they are not relevant.
  • Addresses: Link(s) to GH issue(s) addressed. Include KDS links as well as links to related issues in a consumer product repository too.
  • Components: Affected public KDS component. Do not include internal sub-components or documentation components.
  • Breaking: Will this change break something in a consumer? Choose from: yes / no
  • Impacts a11y: Does this change improve a11y or adds new features that can be used to improve it? Choose from: yes / no
  • Guidance: Why and how to introduce this update to a consumer? Required for breaking changes, appreciated for changes with a11y impact, and welcomed for non-breaking changes when relevant.

[#PR no]: PR link

Steps to test

I suggest looking at these issues https://github.com/learningequality/kolibri-design-system/issues/530 https://github.com/learningequality/kolibri-design-system/issues/528 https://github.com/learningequality/kolibri-design-system/issues/529

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

• [x] Contributor has fully tested the PR manually
• [ ] If there are any front-end changes, before/after screenshots are included
• [ ] Critical and brittle code paths are covered by unit tests
• [x] The change is described in the changelog section above

Reviewer guidance

• [ ] Is the code clean and well-commented?
• [ ] Are there tests for this change?
• [ ] Are all UI components LTR and RTL compliant (if applicable)?
• [ ] Add other things to check for here

After review

• [ ] The changelog item has been pasted to the CHANGELOG.md

Comments

[AllanOXDi]

I've added a few cards to the playground for @radinamatic to perform QA testing. Once everything is approved by the QA team, we'll remove the cards before merging the code.

[radinamatic]

Overall very good implementation, you followed all the recommended accessibility guidelines 👏🏽 💯 🙂

One thing that needs to be corrected is the use of the semantic header and footer elements inside cards. We should have been more clear in the component specification, but the fact that we call something card footer and card header does not imply that semantic header and footer landmark elements need to be implemented. These are page-level elements and having them (announced) for each of potentially tens of cards on the page would be confusing.

Header	Footer

[radinamatic]

Regarding the responsiveness, I don't know if it's because of Netlify view of the playground (🙃) but I'm wondering if it's how it's supposed to look and behave...? Or maybe we can only properly test this once the cards are in the grid? 🤔

https://github.com/learningequality/kolibri-design-system/assets/1457929/faa54f4a-6f99-4643-8967-0ad6248af5a0

[radinamatic]

The last question I had was regarding the other interactive elements inside the card (button/icon to open other options, etc.): that is not in the scope of this PR and is supposed to come in subsequent ones, correct?

[AllanOXDi]

The last question I had was regarding the other interactive elements inside the card (button/icon to open other options, etc.): that is not in the scope of this PR and is supposed to come in subsequent ones, correct?

I think it's something we tried and tested here https://github.com/learningequality/studio/pull/4480

[AllanOXDi]

but I'm wondering if it's how it's supposed to look and behave...? Or maybe we can only properly test this once the cards are in the grid? 🤔

I think that's how it should behave for now.

Regarding the responsiveness, I don't know if it's because of Netlify view of the playground (🙃) but I'm wondering if it's how it's supposed to look and behave...? Or maybe we can only properly test this once the cards are in the grid? 🤔

Untitled.Project.mp4

The Grid will control it's behavior further . I will defer to @MisRob for more thoughts and guidance on this.

[radinamatic]

I think it's something we tried and tested here learningequality/studio#4480

I don't recall having tested that, is it live somewhere for me to take a look?

[AllanOXDi]

I don't recall having tested that, is it live somewhere for me to take a look?

Not- it's not yet live for testing as dev is still on going.

[MisRob]

Thank you @radinamatic and @AllanOXDi.

Regarding responsiveness - that's something that a grid will control, so yes, it is to be implemented and tested later on.

Regarding buttons in the footer area - KCard itself only provides place for them. It is products that will be responsible for adding buttons, context menus, etc. and ensuring it doesn't break a11y. We tried it a bit in a Studio PR as @AllanOXDi mentioned, but that's work in progress and there should be more thorough QA any time we add new cards in a product. After we start seeing some patterns, we may also consider adding some guidance to the documentation or adding features to KCard to further support such cases.

[radinamatic]

All good then, will wait for the refactor of semantic header and footer landmark elements here to approve, and looking forward to testing the PRs for follow up steps of the new KCard! 🙂

[MisRob]

[required] title slot is to be an alternative to the title prop, however now the slot renders in addition to the title slot and is not showing in a correct place. For example, for

<KCard
    :to="{ name: '/' }"
    :headingLevel="2"
    layout="vertical"
  >
    <template #title>
      Card title
    </template>
</KCard>

I see the title text not being rendered as heading:

Could you adjust the behavior to follow the expected behavior in all of the following cases? Note that in one of them it works well already, but not for all - including all for clarity. There's also an additional issue in the screenshots below regarding the placement of the aboveTitle but I believe it is already being addressed.

	Expected
only title prop <KCard title="Card title">
only title slot <KCard><template #title>Card title</template></KCard>
both title prop and title slot <KCard title="Card title from the prop"><template #title>Card title from the slot</template></KCard> - give preference to the title prop and ignore the slot

[MisRob]

[required] One important area to address here is to check the resulting UI against the "Updated Cards Specs Metadata" designs and make sure that all the logic and styles that KCard is responsible for aligns to them (while leaving out areas that KCard is not responsible for, such as content of slots like icons, buttons, etc.) Few examples:

Design	Implementation

These are just a few places I noticed, and is not meant to be a full list. Could you please take some time to test all available layouts and compare with designs? Placement, font sizes, padding/margins, and pretty much anything we can do to follow designs closely. Figma will provide you with all information and useful measurement tools, such as

Note that letterboxing should be preserved though even though it's not showed on the designs. The image on the designs corresponds to the whole thumbnail area (possibly letterboxed) in the final implementation.

[MisRob]

@AllanOXDi There are some acceptance criteria from the issues this PR is meant to close that appear not yet be implemented or have unclear status. Could you please read through all related issues, particularly "Markup/style requirements" and the "Acceptance criteria" sections and resolve them or mention where things are if there are any blockers? Some examples: "Supports RTL", "Even though the whole area is clickable, text content is still selectable", "[documentation] contains guidance on the headingLevel to notify developers that they need to choose a correct level based on context", and perhaps few more.

I know it's lots of work, but there's time to finish it and would suggest we do so in this PR since it's not draft like the first Studio PR. I think it will be easier than opening many follow ups since there's no pressure around merging. This will prepare robust ground for further work and help with implementing card grids as well. If you needed to chat around how to organize work or encountered some blocking issues, we'll have time to chat :)

[MisRob]

@AllanOXDi I think I've finished the first round. At this point, I focused rather on higher-level issues. I will review one more time after this feedback as well as feedback from @radinamatic and @AlexVelezLl is addressed since there will be some important changes. Thanks a lot for this work, it's shaping up really great!

[MisRob]

For the reviewers, just a note that resolving the first PR feedback is still in progress, so the time for re-review will come later.

[MisRob]

Note that RTL will be done as part of https://github.com/learningequality/kolibri-design-system/issues/665 rather than in this PR

[MisRob]

Hi @AllanOXDi, I finished the final review. Overall it feels much more robust now, layouts look great, and I really appreciate you took time to prepare cards preview similar to the cards we will implement in Kolibri. I am sure this will make all further work in products smooth. Thank you for all your hard work :)

As we chatted earlier, it'd be great to have this merged before you're back online and open follow-ups if there's some more work. I will open a few, but for some changes, it's a bit easier for me to push them right away rather than explaining in new issues, and I also hope it will help with other work we have in progress in Kolibri and Studio that you can return to when you are back. I hope you don't mind - it's done with the intention of support, and hope it aligns well with all we talked about together. I'm trying my best to describe reasons for changes in the commit messages, and will be available to talk about each of them. The main areas updated I considered important (cc @AlexVelezLl for review):

User-facing:

• Fixes for some last issues with the title slot
• Consistency of the title styles no matter if title provided via prop or slot
• Improvements to styles robustness and content tolerance, and some last styling touches
• Ensuring that thumbnails can display placeholders via "thumbnailPlaceholder" slot when thumbnail is not available

Other:

• Re-organization of styles in KCard regarding various layouts so that styles belonging to particular layouts are grouped together
  • This is really not so important for users, however I believe it will help with future development, and it also helped me a lot with some styling updates I am mentioning above. I think we had this done already to some extent as response to @AlexVelezLl's first review, but seems to have gotten lost with big styles overhaul after fixes to markup structure. It's lots of changes but rather organizational with a few fixes applied, the foundational techniques you put in place stay the same.

(You will also see some documentation and renaming mixed in my commits and that's just me nitpicking and doing little improvements when I see an opportunity as I'm in the code already, nothing really important, and in majority of cases, I wouldn't be posting to review :)

After I am done, I will ask @AlexVelezLl to review my updates, and then we will merge. Thanks both and talk to you soon.

[MisRob]

Would you please give the components one last look, @AlexVelezLl? Please also see the playground page that simulates more use-cases now. I will delete it before merging, but keep a copy somewhere for testing (this will be perfect first task for automated visual tests :)

Also please note that the following will be resolved in follow-ups (some already open, and I will open later remaining ones). If there's any feedback from you that's not resolved and not blocking, please mention.

• Clarify how cards will behave in a grid regarding their height with designers, e.g. should the cards height be always the same or should the row adjust to the highest card (current implementation is my best guess)? Based on that, determine if we need to be able to override cards heights/widths, remove spaces occupied by empty slots, etc. (I will do this as part of grid work)
• Some more guidelines in documentation for frequent tasks (probably me)
• Vertical/horizontal layout with no thumbnail (me or Allan)
• Adding constants for layout and thumbnaiDisplay and related prop validators (Allan)
• Implementing RTL support (Allan)

[radinamatic]

I took a look at the latest version of the playground and tested with NVDA in Windows.

One thing popped up as a concern, but again I'm not sure if it's determined by this base card implementation, or will it be further injected by other products. Namely, the color contrast of the channel name (is that in the aboveTitle slot?) is insufficient:

Similarly, even though I understand what was mentioned earlier, that:

products that will be responsible for adding buttons, context menus, etc. and ensuring it doesn't break a11y.

I just wanted to reiterate 🙂 , that we need to pay attention to offer full and comprehensive information about those element in the footer. For example, this is the full readout of one of the cards by NVDA:

clickable  
<Title>  
link    
heading    level 2  
<Below title>
PracticeShort Activity  20

All above looks Ok, just 2 comments regarding the last line:

• a separation is missing between the type of resource and the duration
• progress element needs a role and a label, otherwise the screen reader outputs just the number (20) without any context

I know this might not be in the scope of the present PR, but it cannot hurt to repeat it 😉

[MisRob]

Thank you @radinamatic, I appreciate it. Yes, these are all the things that consuming apps need to ensure as they will inject it. I'd like us to have some guidance into the documentation to clarify for developers what we need to take care of when implementing cards on top of this component vs what is it already taking care of. Also, as soon as we have first few Kolibri card components implemented and a11y tested, we could use the way they look for the documentation examples to reflect this all a bit better.

[MisRob]

^ We will keep working on this all gradually as part of the project, this component is a core step, and also a beginning of larger body of work in a sense.

[MisRob]

Thanks for feedback @AlexVelezLl! Merging now.