Use Accessible Cards in Lesson Resource Selection #13060

nucleogenesis · 2025-02-08T00:06:32Z

Summary

Introduces a component MetadataChips in kolibri-common to handle displaying lists of metadata chips
Refactors useCoachMetadataTags to return structured data that slots into MetadataChips nicely
Updates Accessible*Cards to better align w/ the Figma spec and per notes given by @jtamiace last week
Adds bookmark save/remove messages to core strings

Bonus

Updates the api spec export script where it would fail to parse directories that had . in them. (thanks @rtibbles for the help on this!)

DOES NOT INCLUDE

The on-hover "view more" pop-up dealy on channel cards

References

Closes #12732

Reviewer guidance

Code review

Please give particular focus to MetadataChips and my updates to useCoachMetadata; any suggestions for readability/flexibility?
I'd appreciate thoughts re: the use of /deep/ w/ KCard - I wonder if perhaps we should consider an appearanceOverrides prop on them? Personally, I found using /deep/ here to be pleasant as the class hierarchies change as, say, the screen dimensions change - so you can easily override styles for each particular configuration of the KCard (vertical vs horizontal, small vs large thumbs, etc).

Design & QA Review

Do things function as expected w/ regard to selecting & deselecting
Be aware that I have NOT fixed this fix bookmarks not fetching when changed #13077 here - so while your bookmark interactions should persist, you may not see the UI reflect reality at times until you reload

github-actions · 2025-02-08T00:39:32Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.18.0a3.dev0_git.117.gf79a11b4.pex
Windows Installer (EXE)	kolibri-0.18.0a3.dev0+git.117.gf79a11b4-windows-setup-unsigned.exe
Debian Package	kolibri_0.18.0a3.dev0+git.117.gf79a11b4-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.18.0a3.dev0+git.117.gf79a11b4.dmg
Android Package (APK)	kolibri-0.18.0a3.dev0+git.117.gf79a11b4-0.1.4-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.18.0a3.dev0+git.117.gf79a11b4.zip
TAR file	kolibri-0.18.0a3.dev0+git.117.gf79a11b4.tar.gz
WHL file	kolibri-0.18.0a3.dev0+git.117.gf79a11b4-py2.py3-none-any.whl

nucleogenesis · 2025-02-11T23:21:31Z

@marcellamaki @AlexVelezLl

The Bookmarks count is not updated on the root page of the LessonResourceSelection when I add/remove bookmarks while navigating the tree. So if I have 4 bookmarks, go in and add 4 more, then I go back to the root and see the bookmarks card still has 4.

I tried to hack at it to fix it a bit but couldn't get it to re-fetch correctly. I gave it like 30 mins and gave up on it - should I make a follow-up for issue for this?

FWIW I tried:

Put the force: true on the ContentNodeResource.fetch_bookmarks call that happens on that page
Call the getBookmarks in SelectionIndex during route update and enter, which didn't work
Combination of these two things
Get the fetchData method from the fetchBookmarks and call it when the page loads (this resulted in infinite page reloads 🤷‍♂️ )
I threw other things at the wall with no luck 😅

It seems like it might be some part of how the fetchBookmarks object works - I expected the fetchData method to be the one that'd work so maybe I'm missing something there.

AlexVelezLl · 2025-02-13T18:27:22Z

Hey @nucleogenesis! Yes, we will need to call the bookmarksFetch.fetchData right after we save or remove a bookmark or if we dont want to make another api call here to load them, we can also add/remove items from the bookmarksFetch.data ref array and update the bookmarksFetch.count value. Any of them will properly update the bookmarks list.

Get the fetchData method from the fetchBookmarks and call it when the page loads (this resulted in infinite page reloads 🤷‍♂️ )

The reason of the infinite page reload here is because fetching bookmarks triggers this loading computed property to true, and this causes a remount of the router view here. And thats why if we call it inside the setup method it will cause an infinite loop of loading -> fetching -> loading -> fetching, etc. A solution for this could be to replace this v-else with a v-show so the component is not unmounted, but I dont think fetching the bookmarks all the times that we open the select from bookmarks page would be very efficient, and also because we also need the bookmarks updated in the SelectionIndex page because there we also render the bookmarks count, so we would also need to call it inside that subpage.

I think this can be a follow up issue, and we can explore more there the implications of each option :)

AlexVelezLl · 2025-02-13T18:42:40Z

kolibri/plugins/coach/assets/src/views/lessons/LessonResourceSelectionPage/ContentCardList.vue

+        });
+      }
+
+      function isBookmarked(contentnode_id) {


Just wonder if it would be better to check the existence of content.bookmark object to verify if the resource has been bookmarked? For example we use this to show the "bookmarked x days ago" in the SelectFromBookmarks subpage here.

With this we avoid needing to load all bookmarks each time this component is mounted. Because this component will be remounted each time we navigate through the folder tree/bookmarks/search results and I think we can save us from making this fetch all these times.

Thanks @AlexVelezLl - I'll ping you as I work on it in more detail addressing this issue in another PR

AlexVelezLl · 2025-02-13T18:56:08Z

kolibri/plugins/coach/assets/src/views/lessons/LessonResourceSelectionPage/ContentCardList.vue

+        @toggleBookmark="toggleBookmark"
+      >
+        <template #belowTitle>
+          <p v-if="contentCardMessage(content)">{{ contentCardMessage(content) }}</p>


I found a little regression here. In the SelectFromBookmarks component, the cards now dont show the Bookmarked label:

Before:

After:

Great catch! Fixed

AlexVelezLl

Thanks @nucleogenesis! Nothing blocking, Just some thoughts :)

AlexVelezLl · 2025-02-26T21:33:12Z

kolibri/plugins/coach/assets/src/views/lessons/LessonResourceSelectionPage/ContentCardList.vue

-        </li>
-      </KRadioButtonGroup>
-    </ul>
+          <!-- As a fallback, if any contents have checkboxes at all, we'll want to


Just wondering if we can somehow solve this in the scope of KCard/KCardGrid, not now, but if we see more cases like this I think it can be worth trying cc: @MisRob.

Yes, whatever further improvements we can do to KDS card and grid it'd be lovely to have it reported to KDS, even if it'd be for tracking purposes at first

AlexVelezLl · 2025-02-26T21:38:33Z

packages/kolibri-common/components/Cards/AccessibleFolderCard.vue

    thumbnailAlign="right"
  >
    <template #thumbnailPlaceholder>
      <div class="default-folder-icon">
        <KIcon
          icon="topic"
          :color="$themePalette.grey.v_700"
+          style="top: 0"


I have also found myself setting this top value to 0 😅. Its a little annoying imo.

Very annoying 😄

AlexVelezLl · 2025-02-26T21:43:19Z

kolibri/plugins/coach/assets/src/views/lessons/LessonResourceSelectionPage/ContentCardList.vue

@@ -228,4 +284,17 @@
    margin-left: $checkbox-offset;
  }

+  .filter-chip {


I am not finding any node that uses these classes, probably they were left behind after some refactoring 😅

AlexVelezLl · 2025-02-26T21:50:23Z

packages/kolibri-common/composables/useCoachMetadataTags.js

+  };
+
+  const getResourceTags = () => {
+    return [...getActivityTags(), ...getDurationTag(), ...getCategoryTags()];


It seems that getActivityTags is expected to return an array, but if we have contentNode.learning_activities.length > 1 then we will return a single object instead: https://github.com/learningequality/kolibri/pull/13060/files#diff-485e19c293664fde10e23bc0b3d37eb38fd664f996248cb729a31964d08d5558R55.

AlexVelezLl · 2025-02-26T21:57:01Z

packages/kolibri-common/composables/useLearningActivities.js

@@ -0,0 +1,190 @@
+/**


This is a file rename, right? Should we remove the useLearningActivities that exists in Learn and refactor the import references or is not needed for now?

Thank you for catching this! I just moved it so I could use it in my work but skipped the removal of the one in learn and updating all of the imports for it throughout Learn 😓

It's fine to not do that refactor in Learn now, but @nucleogenesis can you make a follow up issue for this and tag it into Planned Patch 1? I think this would be good cleanup to do promptly, although not release blocking, to avoid confusion

Made #13197 for this

AlexVelezLl · 2025-02-27T00:01:25Z

packages/kolibri-common/components/Cards/AccessibleFolderCard.vue

  };

 </script>


 <style lang="scss" scoped>

-  .header-bar {
+  /deep/ .k-horizontal-with-small-thumbnail.k-thumbnail-align-right .k-thumbnail {
+    margin: 0;


I think this should be fixed in KCard, its a little inconsistent that when we have thumbnailDisplay=small the thumbnail is bigger than when we have thumbnailDisplay=large 😅.

thumbnailDisplay small (+margin: 0) thumbnailDisplay large

If we want to keep the combination of small thumbnailDisplay + margin 0 to accomodate better the thumbnail for now, we will also need to set the border radius of top-left and bottom-left corners to 0.

@AlexVelezLl I'd love to apply these updates (where appropriate) to KCard but I hesitated to get into changing KDS if I could avoid it for now.

That said, @MisRob if you could give a look here to some of the /deep/ styles I used that'd be really great.

The issue I ran into was that when I used small then the footer extended to the right, underneath the thumbnail. We want the footer to stop at the thumbnail (ie, it's the same width as title & description) -- but also for the thumbnail to be smaller.

The changes I've used kind of resize everything to accommodate the specs.

I've updated the border-radius, great catch thanks Alex!

The issue I ran into was that when I used small then the footer extended to the right, underneath the thumbnail. We want the footer to stop at the thumbnail (ie, it's the same width as title & description) -- but also for the thumbnail to be smaller.

This sounds to me as possible inconsistency between designs and KCard implementation, or between the many designs, we may need to talk about. Let's chat on Slack so I can understand this better first.

jtamiace · 2025-02-27T00:10:12Z

Looking great @nucleogenesis 👏🏼
Only have a few notes on polishing the layout

Could you make the distance between the title and labels on resource cards match the distance that it is on folder cards?
I'm not sure how much margin is in between each of the cards, but I think that can also do with being reduced a bit. In Figma I have that set to 24px.
The activity label on resource cards could use a little more left padding so that it matches the right - a bit squeezed right now.
The hover state of the bookmark icon looks squished vertically, and I notice that the tooltip label for the empty bookmark said "Saved from bookmarks" while in the channel folder

MisRob · 2025-02-28T09:33:28Z

Hi @nucleogenesis, this is wonderful overall. Regarding the overrides where you mentioned me, I posted to Slack to clarify few things at first, before I dive into details.

Regarding @jtamiace:

I'm not sure how much margin is in between each of the cards, but I think that can also do with being reduced a bit. In Figma I have that set to 24px.

This should be possible to configure by rowGap/columnGap https://design-system.learningequality.org/kcardgrid#layout-customization :)

By the way, @jtamiace , would you say that 24px is the best general value that we should use as default on all KCardGrids? Default is 30px now, can be seen on live examples.

MisRob · 2025-02-28T09:59:21Z

I think it's important to clarify styles overrides, but I'd be fine if actual changes, if needed, be done as follow-up.

Besides that, I gave the card components very brief skim and didn't notice anything major. So nothing blocking from me. Lovely to see it coming together :)

jtamiace · 2025-02-28T23:07:03Z

@MisRob thanks, nice to know that's built in and yes I think 24px feels pretty balanced

pcenov · 2025-03-04T14:57:40Z

Hi @nucleogenesis, looks great, I just have the following questions:

For all of my folders the description text is not shown on the card under the title as specified in Figma, is this intentional or an oversight:

no-folder-description.mp4

In Figma I can see an info icon displayed next to the bookmark icon but it's not implemented yet here or perhaps it was decided that it shouldn't be added?
When I search for a resource, if there are more than 25 results then the "View more" button is not positioned correctly - probably not caused by your changes here?

In the search results I see a mixture of folders and cards - is this the expected behavior? Also some cards are wider than others - shouldn't the cards be of the same size:

radinamatic · 2025-03-04T15:06:27Z

Did not find any glaring issues in functional and visual aspects of the lesson resource selection, but navigating by keyboard was somewhat frustrating... 😞

I realize that some of these may be out of the scope of the present PR, but I thought that we would have adopted the proper coding strategies by now, and encountering them again just added to the frustration:

Sidebars MUST behave as full page modals (in mobile views they really are) and trap the focus within the sidebar until the user closes it.
If the user workflow contains several views within the sidebar (for example while navigating back and forth through the folders), focus order should be logical: when user selects one folder and presses Enter to open and navigate within, the focus in the next step (view) should be placed on top of the sidebar (maybe the Search button), and not at the bottom (the Save & finish button).
https://github.com/user-attachments/assets/f430fc29-220b-4733-a92e-bc721dcb4246

Now for the issues that do need to be fixed for this feature to be considered accessible:

Top left back button (left arrow icon) is missing the label (Go back should suffice).
Select all checkbox is missing the visual focus outline when the state is half/partially checked.
https://github.com/user-attachments/assets/8032b5e8-41e8-4cb7-82a9-f68d4ec7e223
When one or more resources are selected, the link that appears at the bottom of the sidebar (N resources selected (...KB)) must be announced through the aria-live="polite" region.
Bookmark button still needs some a11y TLC:
- when the resource is NOT bookmarked, button should have the label Save to bookmarks, instead of the Saved from bookmarks (string I'm not sure what does it even mean)
- when user clicks the button to bookmark the resource, that change of state is displayed visually by different icon, but those who cannot see are missing the action. We need to add the message/string Saved to bookmarks (can be visually hidden) that must also be announced through the aria-live="polite" region
- when the user has bookmarked the resource, the label for the button must change to Remove from bookmarks
- if/when the user decides to un-bookmark it, that change of state must also be announced by passing the Removed from bookmarks message through the aria-live="polite" region, alongside the icon change (string should probably need to be added to commonCoreStrings)
Maybe changing the Saved from bookmarks string that is not being used anywhere (and I have had it hidden on Crowdin for the last 2 releases) into Saved to bookmarks would help with this last point 🙂

marcellamaki

Approving 🎉 with follow up in KCard to come in planned patch 1 and KDS

…to be added)

github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Feb 8, 2025

marcellamaki self-assigned this Feb 11, 2025

nucleogenesis requested review from jtamiace, marcellamaki and AlexVelezLl February 11, 2025 22:59

nucleogenesis force-pushed the 12732--refactoring-to-use-updated-cards branch from 48e8913 to ffb7871 Compare February 11, 2025 23:12

nucleogenesis marked this pull request as ready for review February 11, 2025 23:12

nucleogenesis mentioned this pull request Feb 11, 2025

Update the 'Bookmarks' card (i.e. to click into the bookmarks selection) uses the new folder card or KCard #12694

Open

nucleogenesis mentioned this pull request Feb 12, 2025

fix bookmarks not fetching when changed #13077

Open

marcellamaki mentioned this pull request Feb 13, 2025

Manage lesson resources > Exercises - Missing metadata #13075

Open

AlexVelezLl reviewed Feb 13, 2025

View reviewed changes

nucleogenesis force-pushed the 12732--refactoring-to-use-updated-cards branch 4 times, most recently from 5dd8993 to ee27359 Compare February 25, 2025 21:49

github-actions bot added the DEV: backend Python, databases, networking, filesystem... label Feb 25, 2025

nucleogenesis requested a review from pcenov February 25, 2025 23:36

AlexVelezLl reviewed Feb 27, 2025

View reviewed changes

marcellamaki mentioned this pull request Mar 4, 2025

Address a11y bugs in side panel/resource navigation in lessons and quizzes #13145

Open

marcellamaki approved these changes Mar 13, 2025

View reviewed changes

nucleogenesis added 26 commits March 13, 2025 08:56

use Accessible*Cards in LessonResourceSelectiionPage

3751ce4

remove info icon from kcards in lesson resource selection

9c58ba9

handle bookmarking in lesson resource selection KCards

39d412d

include included_* fields in ChannelMetadata

eb11312

improved resource/folder/channel metadata on cards & style updates

32eac77

pass slot to ensure bookmarked-ago message shown

759d2c6

style fixes galore

5057072

set default first-el-focus in SidePanelModal

05f31c0

style fixes for cards; include desc in folders

1fab19a

use prop overrides in KCardGrid to set gaps

a75cd6b

revert overriding styles

a807836

improve hard-coded spacing

5ab4914

use bookmarks language for btns & aria polite notifications (strings …

8d9cfc7

…to be added)

use path.parse & path.join in globbing rather than split

1c79373

add strings for bookmarks

d26a458

add polite aria message when resource selection changes

6494317

inclue backaction in backb utton in resource side panel

ed5b0c2

fix view-more button in search results

2c0bbde

fix broken folder icon size

fda85de

remove file left behind in rebase

13f182d

fix weird wideness of text bug

14bc4ab

fetch bookmarks whenever we enter the SelectionIndex route

9e3c315

fix spacing in folder

0dc719d

send polite message on (de)select

7af6d4c

remove unused code in coach metadata tags

26f5a9b

include all tags & lang tags for channels; move logic over

49fbdd8

nucleogenesis force-pushed the 12732--refactoring-to-use-updated-cards branch from 56cd92a to 49fbdd8 Compare March 13, 2025 15:57

nucleogenesis merged commit 9082dda into learningequality:develop Mar 13, 2025
36 checks passed

nucleogenesis mentioned this pull request Mar 13, 2025

Fix: Show search & setting buttons at channel level in quiz resource selection #13198

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Accessible Cards in Lesson Resource Selection #13060

Use Accessible Cards in Lesson Resource Selection #13060

nucleogenesis commented Feb 8, 2025 •

edited

Loading

github-actions bot commented Feb 8, 2025 •

edited

Loading

nucleogenesis commented Feb 11, 2025 •

edited

Loading

AlexVelezLl commented Feb 13, 2025 •

edited

Loading

AlexVelezLl Feb 13, 2025

nucleogenesis Feb 25, 2025

AlexVelezLl Feb 13, 2025

nucleogenesis Feb 25, 2025

AlexVelezLl left a comment

AlexVelezLl Feb 26, 2025

MisRob Feb 28, 2025

AlexVelezLl Feb 26, 2025

nucleogenesis Feb 27, 2025

AlexVelezLl Feb 26, 2025

AlexVelezLl Feb 26, 2025

AlexVelezLl Feb 26, 2025

nucleogenesis Feb 28, 2025

marcellamaki Mar 12, 2025

nucleogenesis Mar 12, 2025

AlexVelezLl Feb 27, 2025

nucleogenesis Feb 27, 2025

nucleogenesis Feb 27, 2025

MisRob Feb 28, 2025

jtamiace commented Feb 27, 2025

MisRob commented Feb 28, 2025

MisRob commented Feb 28, 2025

jtamiace commented Feb 28, 2025

pcenov commented Mar 4, 2025

radinamatic commented Mar 4, 2025 •

edited

Loading

marcellamaki left a comment

Use Accessible Cards in Lesson Resource Selection #13060

Use Accessible Cards in Lesson Resource Selection #13060

Conversation

nucleogenesis commented Feb 8, 2025 • edited Loading

Summary

Bonus

DOES NOT INCLUDE

References

Reviewer guidance

Code review

Design & QA Review

github-actions bot commented Feb 8, 2025 • edited Loading

nucleogenesis commented Feb 11, 2025 • edited Loading

AlexVelezLl commented Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexVelezLl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtamiace commented Feb 27, 2025

MisRob commented Feb 28, 2025

MisRob commented Feb 28, 2025

jtamiace commented Feb 28, 2025

pcenov commented Mar 4, 2025

radinamatic commented Mar 4, 2025 • edited Loading

marcellamaki left a comment

Choose a reason for hiding this comment

nucleogenesis commented Feb 8, 2025 •

edited

Loading

github-actions bot commented Feb 8, 2025 •

edited

Loading

nucleogenesis commented Feb 11, 2025 •

edited

Loading

AlexVelezLl commented Feb 13, 2025 •

edited

Loading

radinamatic commented Mar 4, 2025 •

edited

Loading