Use Accessible Cards in Lesson Resource Selection #13060
Conversation
github-actions
bot
added
APP: Coach
Build Artifacts
|
nucleogenesis
force-pushed
the
12732--refactoring-to-use-updated-cards
branch
from
48e8913 to
ffb7871
Compare
|
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:
It seems like it might be some part of how the |
|
Hey @nucleogenesis! Yes, we will need to call the
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 I think this can be a follow up issue, and we can explore more there the implications of each option :) |
| }); | ||
| } | ||
|
|
||
| function isBookmarked(contentnode_id) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks @AlexVelezLl - I'll ping you as I work on it in more detail addressing this issue in another PR
| @toggleBookmark="toggleBookmark" | ||
| > | ||
| <template #belowTitle> | ||
| <p v-if="contentCardMessage(content)">{{ contentCardMessage(content) }}</p> |
There was a problem hiding this comment.
I found a little regression here. In the SelectFromBookmarks component, the cards now dont show the Bookmarked label:
Before:
After:
There was a problem hiding this comment.
Great catch! Fixed
nucleogenesis
force-pushed
the
12732--refactoring-to-use-updated-cards
branch
4 times, most recently
from
5dd8993 to
ee27359
Compare
github-actions
bot
added
the
DEV: backend
There was a problem hiding this comment.
Thanks @nucleogenesis! Nothing blocking, Just some thoughts :)
| </li> | ||
| </KRadioButtonGroup> | ||
| </ul> | ||
| <!-- As a fallback, if any contents have checkboxes at all, we'll want to |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| thumbnailAlign="right" | ||
| > | ||
| <template #thumbnailPlaceholder> | ||
| <div class="default-folder-icon"> | ||
| <KIcon | ||
| icon="topic" | ||
| :color="$themePalette.grey.v_700" | ||
| style="top: 0" |
There was a problem hiding this comment.
I have also found myself setting this top value to 0 😅. Its a little annoying imo.
| @@ -228,4 +284,17 @@ | |||
| margin-left: $checkbox-offset; | |||
| } | |||
|
|
|||
| .filter-chip { | |||
There was a problem hiding this comment.
I am not finding any node that uses these classes, probably they were left behind after some refactoring 😅
| }; | ||
|
|
||
| const getResourceTags = () => { | ||
| return [...getActivityTags(), ...getDurationTag(), ...getCategoryTags()]; |
There was a problem hiding this comment.
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.
| @@ -0,0 +1,190 @@ | |||
| /** | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😓
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Made #13197 for this
| }; | ||
|
|
||
| </script> | ||
|
|
||
|
|
||
| <style lang="scss" scoped> | ||
|
|
||
| .header-bar { | ||
| /deep/ .k-horizontal-with-small-thumbnail.k-thumbnail-align-right .k-thumbnail { | ||
| margin: 0; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I've updated the border-radius, great catch thanks Alex!
There was a problem hiding this comment.
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.
|
Looking great @nucleogenesis 👏🏼
|
|
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:
This should be possible to configure by By the way, @jtamiace , would you say that 24px is the best general value that we should use as default on all |
|
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 :) |
|
@MisRob thanks, nice to know that's built in and yes I think 24px feels pretty balanced |
|
Hi @nucleogenesis, looks great, I just have the following questions:
no-folder-description.mp4
|
|
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:
Now for the issues that do need to be fixed for this feature to be considered accessible:
|
nucleogenesis
force-pushed
the
12732--refactoring-to-use-updated-cards
branch
from
6c60ab4 to
1b8d71b
Compare
|
I've found this bug where some files are way too wide. I can't figure out why exactly this is happening, but it seems related to how the thumbnails are sized somehow (although they all just are 100% width/height). I played w/ flex properties and some /deep/ styles and nothing came to me that resolved it well. I'll come back to it again later on - but perhaps this is something that needs to be addressed in KDS or in a follow-up? (cc @marcellamaki)
|
|
Results of the latest round of testing with today's asset (I'm adding the feedback to #13145 so anything still pending can be worked on there, and not blocking the merge of this PR):
|
|
Agreed with @MisRob on setting limits on card sections to manage the total height of the card. I actually think that as long as there are the maximum limits for those sections, it's okay that there's a little bit of height variability when the cards are stacked vertically. Making the heights match seems more relevant for when there is >1 card in the same row? Although, I can foresee needing to introduce limits to the number / rows of labels in the future since I don't think there is one at the moment. I think we should do this eventually since it's possible for the user to add multiple labels for categories, activity, etc. on the Studio side. @nucleogenesis also, would you be able to make the folder label appear closer to the title? Similar to the change that you made for the resource card labels earlier. |
|
@AlexVelezLl I tried putting the Perhaps a future enhancement could be to provide methods for adding/removing bookmarks that do the client-side data updating for us to avoid the API request. However, it's not a very heavy request so hopefully we don't see that it adds much overhead. |
|
@jtamiace how's this?
Screencast_20250311_152700.webm
I tried to work out why the select all isn't showing the outline but I couldn't figure it out. The CSS styles are being applied to the element as expected... but just for some reason it isn't visible that very particular case 😞 Should make that a follow-up bug - hopefully someone w/ fresh eyes on it can find a solution in the next couple weeks. |
|
| @@ -0,0 +1,190 @@ | |||
| /** | |||
There was a problem hiding this comment.
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
| const { windowBreakpoint } = useKResponsiveWindow(); | ||
| // A little data massaging to make the metadata passable to useCoachMetadataTags |
There was a problem hiding this comment.
Similarly, I'm wondering if (in follow up), it might be appropriate to update useCoachMetadataTags to manage this there, rather than within the component. We can discuss and perhaps create a follow up issue, but it seems like it might make sense to move this over.
There was a problem hiding this comment.
Yeah could probably do this stuff in the getChannelTags func
| @@ -1,66 +1,126 @@ | |||
| //import { availableLanguages } from 'kolibri/utils/i18n'; | |||
| //import uniq from 'lodash/uniq'; | |||
| ...getActivityTag(), | ||
| ...getDurationTag(), | ||
| ...getActivityTags(), | ||
| //getDurationTag(), |
There was a problem hiding this comment.
Are we not using the duration anywhere?
There was a problem hiding this comment.
oof I meant to remove all of this stuff altogether as I replaced it w/ the get*Metadata functions.
There was a problem hiding this comment.
@marcellamaki I've cleaned up unused stuff here.
When I spoke w/ @jtamiace the tags that are returned by the get*Metadata functions are the ones she said we should be returning. I left a couple internal functions in comments as they may be useful in the future if we make design changes.
Thanks, it looks good |
Summary
MetadataChipsinkolibri-commonto handle displaying lists of metadata chipsuseCoachMetadataTagsto return structured data that slots intoMetadataChipsnicelyBonus
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
References
Closes #12732
Reviewer guidance
Code review
/deep/w/ KCard - I wonder if perhaps we should consider anappearanceOverridesprop 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