-
Notifications
You must be signed in to change notification settings - Fork 647
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
Resource discovery: Implement folder and resource card #12418
base: develop
Are you sure you want to change the base?
Resource discovery: Implement folder and resource card #12418
Conversation
Build Artifacts
|
d4b1a45
to
9bba53c
Compare
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.
Hi Allan, thanks :) I appreciate clarity in the PR description in regard to review and also what's done and what's coming, helpful. I hope to have all issues I promised to open ready soon. Cards look very good, and their API interface makes sense to me. I am leaving a couple of comments from the first code walkthrough.
@@ -37,7 +37,13 @@ | |||
disabled, tabindex, is-leaf class set here to hack making the card not clickable | |||
if you're trying to make the card clickable remove these properties | |||
--> | |||
<AccessibleFolderCard | |||
v-if="content.kind === 'topic'" |
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.
Not so important as it's temporary change, rather just fyi for future work there's constants available here
kolibri/kolibri/core/assets/src/constants.js
Lines 28 to 42 in df0c4ee
export const ContentNodeKinds = { | |
AUDIO: 'audio', | |
DOCUMENT: 'document', | |
VIDEO: 'video', | |
EXERCISE: 'exercise', | |
TOPIC: 'topic', | |
HTML5: 'html5', | |
CHANNEL: 'channel', // e.g. a root topic | |
EXAM: 'exam', | |
LESSON: 'lesson', | |
CLASSROOM: 'CLASSROOM', | |
ACTIVITY: 'ACTIVITY', | |
SLIDESHOW: 'slideshow', | |
BOOKMARK: 'bookmark', | |
}; |
kolibri/plugins/coach/assets/src/views/plan/LessonResourceSelectionPage/ContentCardList.vue
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/AccessibleFolderCard.vue
Outdated
Show resolved
Hide resolved
<KCard | ||
class="card" | ||
:to="to" | ||
:headingLevel="2" |
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.
[required] I'd suggest that <AccessibleFolderCard>
has a headingLevel
prop and just passes its value to KCard
via the same prop. Reason being that <AccessibleFolderCard>
will be used from more pages in Kolibri, and on each page it can be a member of a different level. This will ensure that devs always make sure to configure it explicitly and correctly in the context of a given page.
> | ||
<template #thumbnailPlaceholder> | ||
<div> | ||
<CardThumbnail |
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.
[required] Regarding the usage of <CardThumbnail>
here, KCard
doesn't need to use another component to re-implement the whole thumbnail area because it already provides it. If we used <CardThumbnail>
here, that would override everything in unintended ways, and it's also unnecessary extra work. Let's leave KCard
to do everything it can and only put folder icon into the #thumbnailPlaceholder
slot. I realize that the slot's name is not intuitive in this regard - can you think of a better name?
(side note for context, ultimately, a big future goal is to get rid of all current card components in Kolibri, <CardThumbnail>
included, in favor of KCard
)
kolibri/plugins/learn/assets/src/views/LibraryPage/AccessibleFolderCard.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/AccessibleFolderCard.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/AccessibleFolderCard.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/AccessibleFolderCard.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/AccessibleFolderCard.vue
Outdated
Show resolved
Hide resolved
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.
Thank you @AllanOXDi! Its super exciting to see KCard already being used in our codebase!! 🎉, and see that the initial implementation has covered the most usecases we had. I left just a couple of thoughts, mostly regarding the html semantic structure 🤗.
kolibri/plugins/coach/assets/src/views/plan/LessonResourceSelectionPage/ContentCardList.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/coach/assets/src/views/plan/LessonResourceSelectionPage/ContentCardList.vue
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/AccessibleFolderCard.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/AccessibleFolderCard.vue
Outdated
Show resolved
Hide resolved
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.
Hi @AllanOXDi thanks for the great work on this!
My main suggestion here is for this PR but also helps us with the "how do we move past a working branch into the 'mainline' work for 0.18 question".
We should move both the ResourceCard and the FolderCard to kolibri-common
. This allows the work to be merged into develop with minimal additional changes, provides the cards for shared use, and also prevents cross-plugin imports (totally fine for this working branch and testing, but not something we want to merge into production. Why no cross-plugin imports? Since it's a plugin architecture, we cannot merge code that assumes the existence of another plugin. Each plugin shouldn't "know" about other plugins that exist, or depend on them. More questions? Richard would love to tell you about it).
Having this implementation as it is, and using the updated version in the content card list is very helpful for manual QA. So, after moving these new components into kolibri-common
, please update the reference for the import in the ContentCardList
for final QA and feedback, so we can make sure everything moved over well.
Once all of the feedback on the cards themselves is resolved, and we confirm with QA that the new cards work as expected in Coach in the ContentCardList
with no regressions, I think we can retarget this PR to develop and merge it directly there. I will then update the related issues in the milestone to be follow up issues that build on top of this foundation. I'm going to think about whether or not we want to include the ContentCardList
update, or if we just want to include the cards being available in kolibri-common
with no actual "placement" of them yet. If anyone has pros/cons, please share.
Also, a bit of a nitpick, but let's have a consistent name for the new cards -- I think making sure it's Accessible_____Card
for each makes sense, so update AccessibilityResourceCard
to be AccessibleResourceCard
.
Great work! I'm so excited that finally KCard
in Kolibri is coming to life!!!! 🎉 🎉 🎉 🎉 🎉 It's been a long time coming, and past Marcella, who struggled through 0.15 card refactoring (and many patch release "fixes" on the card UI ever since), is very grateful!
kolibri/plugins/coach/assets/src/views/plan/LessonResourceSelectionPage/ContentCardList.vue
Outdated
Show resolved
Hide resolved
@AllanOXDi After all feedback here is resolved, and also learningequality/kolibri-design-system#752 finished and linked here, please ping me here - will do one final read through and then we should be ready to |
kolibri/plugins/learn/assets/src/views/LibraryPage/AccessibilityResourceCard.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/AccessibleFolderCard.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/AccessibleFolderCard.vue
Outdated
Show resolved
Hide resolved
4a33906
to
50a4013
Compare
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.
Overall things are looking good! Just a couple non-blocking code questions from me on code review. Great work Allan!
:to="to" | ||
:headingLevel="headingLevel" | ||
layout="horizontal" | ||
thumbnailDisplay="large" | ||
:title="contentNode.title" | ||
:thumbnailSrc="thumbnailSrc" | ||
thumbnailScaleType="centerInside" | ||
thumbnailAlign="right" | ||
:style="{ height: '172px', margin: '16px 0 16px 0' }" |
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.
Non-blocking question:
Would any of these other KCard
props be good to accept as props in AccessibleFolderCard
? Like - would we ever want to use this card in a context where we want a different thumbnail(Align|Display|ScaleType)
or layout
values?
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.
Sorry I missed this comment but I get your point, according to @MisRob suggestion here #12418 (comment), states otherwise.
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.
@nucleogenesis I am certain that scale type will always be the default centerInside
in all card components because their thumbnails are user-uploaded and don't have consistent ratios (you can see more in KImg -> Usage > Scaling).
Regarding the layout related settings, each of the three cards has its layout configuration. As far as we know from the designs, we won't be using multiple configurations for the folder card I commented on, however I can imagine this could easily change depending on the future designs and card type (there's more layouts for the resource card, I believe).
So if it feels important to think through it more carefully, perhaps each prop deserves its own consideration.
In the comment you reference @AllanOXDi, I don't mean to suggest we never use these extra props, but rather that we start using them when there's an actual need. You could argue otherwise depending on a mental model for these cards. I emphasized simplicity at this point, but nothing blocking. You can take it as one of the perspectives on designing interfaces in general and welcome to choose which way to go.
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.
But also at this point we won't be allowing a lot customization of these cards beyond the design. I do remember the reason as to why we allowed customization further using thumbnail(Align|Display)
was handle the edge cases that these cards provide.
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.
These make your point valid @MisRob. Keep it simple and wait until there is a need to
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.
But also at this point we won't allowing a lot customization of these cards beyond the design.
Oh yes, that's true, I just wanted to also acknowledge I see your point in having some customizations. Even though I didn't observe this for the folder card where the discussion originated, later I realized more broadly there's more to it, for example the resource card will likely need to accept more layout
s and thumbnailDisplay
s in close future :) I double-checked designs earlier today to think more about this and noticed
Anyways, good considerations here! You're welcome to put it together and arrive at something that makes most sense to you taking into account all these pieces.
thumbnailSrc: { | ||
type: String, | ||
default: null, | ||
}, |
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.
Non-blocking question:
Is this prop here so that we can override the content's thumbnail value? Is there an expected case of using anything besides contentNode.thumbnail
for the thumbnail here?
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.
Yes , the prop thumbnailSrc is to override the value of the KImg in the KCard. No I think the chances of using anything besides contentNode.thumbnail are very minimal
@@ -0,0 +1,131 @@ | |||
<template> | |||
|
|||
<div> |
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.
[required] Here let's remove this outer <div>
. It's not needed, and more importantly AccesibleResourceCard/KCard
as <li>
will need to be a direct child of KCardGrid
as <ul>
:color="$themePalette.grey.v_600" | ||
:ariaLabel="coreString('savedFromBookmarks')" | ||
:tooltip="coreString('savedFromBookmarks')" | ||
@click="$emit('toggleInfoPanel')" |
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.
Nice, I think at this point it makes sense for this card to just emit on action rather than to contain handling implementation.
[required] I think we will need to rename emitted event names though to something meaningful. For example just toggleBookmark
and for the other emit on line 45 it could be toggleInfo
? I think this should reflect these two icon buttons.
</script> | ||
|
||
|
||
<style lang="scss" scoped> |
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.
[required] Let's cleanup unused classes such as folder-header-bar
and possibly others below as well.
Couple of last notes from me @AllanOXDi, then I think we'll be ready for merge :) |
fe1fd87
to
a59a286
Compare
Done! |
I also manually tested the new resource card now and one thing came out it'd be good to address now and that is when I click the bookmark or info icon buttons, I'm redirected. The expected behavior is rather that the two events get emitted (as already implemented) but we're not redirected to the card's target. I think we already achieved this in Studio somehow? Other than that I think both the folder card and resource card have all they should in the scope of this PR. Regarding a11y QA, as discussed we will do that as part of actual features when the card grid is introduced. |
@AllanOXDi following-up on ^ I assume that |
This is something that I already tried. |
@AllanOXDi would you say a bit more so I can understand what was troubling - best would be to share the concrete code updates. Would you open a trial branch from this one, pushed what you played around with, and shared the link? I can then preview what changes you actually did and help debug. |
Let me do so |
Thank you @AllanOXDi, I will look at it later today if I can spot something :) As |
@AllanOXDi I looked into the issue with |
It's working fine- thanks for the fix |
…rd, which is to navigate to a new route.
6f9b648
to
d71bb85
Compare
Hi @MisRob @AllanOXDi - I have started my review here and have a few thoughts. I'll add some comments and questions tomorrow. Some of them are probably for follow up and some are more general questions as I get more familiar with the relationships between these cards and KCard. Overall it's looking really nice Allan, thanks for all of your hard work! |
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.
Hi @AllanOXDi Thank you for the great work on this. I can really see your increased comfort with all thing Vue components and navigating between KDS components and custom components in our products! I added a couple of comments but they're small.
Aside from the comments, I wanted to raise a little feedback that is outside of the scope of this PR but is interesting for the sake of learning and planning around the mobile view of the cards and this project.
On mobile view of the Folder card, the layout is technically responsive, which is great, but you can see it's a bit crowded.
I realize that the screenshot included in the issue only had more of a "list" view and didn't clearly state that there would be different sizes on mobile. And the image preview in the issue didn't include the mobile level responsiveness specifically. When we actually use this card (beyond just what you have here for the demo), I think we'll want to make sure we use the version of KCard
that is aligned with the spec..
From my understanding talking to @MisRob, it will be simple to do this with a different layout that exists, to be vertical vs. horizontal, which is already supported in KCard
You could try out doing this conditionally where the card is called
<Accessible...Card layout=breakpointLevel === 0 ? "'vertical" : "'horizontal">
but only if you're curious to test it out and see for yourself, as this isn't the final usage and you will be removing that code before merge anyways :)
In upcoming work I'd also encourage you to use a critical/reflective eye when are playing around with testing, and raise questions like "does this look right? did we overlook a mobile display? does this seem like what the designer intended?" Again, this isn't strictly in scope for this PR as the current "use" of the card is for manual QA only. And it's not a criticism of the implementation, but rather some thoughts to help with broader context thinking beyond the checklist of acceptance criteria. Noticing and sharing your perspective on things early, even if it seems like something a bit outside of your task list, is always helpful, and it helps you continue to build those reflective/questioning skills 🙂 And practically speaking, it will also help us make sure we do things like share with the team some of these features of the new KCard
and helps me remember to include these details in upcoming issues I'm writing.
@@ -225,4 +237,9 @@ | |||
margin-left: $checkbox-offset; | |||
} | |||
|
|||
/deep/ .horizontal-with-large-thumbnail { | |||
height: 141px; | |||
margin: 0 0 16px; |
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'm wondering why there's a need to use /deep/
here, and if this is because this is more of an MVP, rather than the full implementation, or if there's something that we're missing with the KCard API. To me this is a small thing but an indicator that something is not quite as flexible as it needs to be, or like we've missed a detail and now are overriding something where there might be a better approach.
This is a bit of a nitpick, but I'm also confused about why the height is 141
px specifically. Was it this in the spec? We usually stick with 4 or 8px increments, or at the very least an even number.
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.
currently, KCard has no set hight and and width- it's something that will be implemented in the gird. So I was just trying to play around with the number to make sure I'm close tot the the design
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 asked @AllanOXDi to not worry too much about these temporary files :) As mentioned this won't be configured on KCard
at all and this whole file will be removed before we merge.
However, it's definitely relevant notes and good guidance. @AllanOXDi it may be be helpful to comment on things like this in the PR comments or code comments so reviewers who don't have context (or just people like me with bad memory :-) can understand things like this.
@@ -25,6 +25,18 @@ | |||
</template> | |||
</component> | |||
</component> | |||
|
|||
<h2>AccessibleResourceCard preview</h2> |
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.
Let's just make sure to remove this before merge
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.
Okay. Once It's approve, I will remove it before merge. I wish I had added a comment
> | ||
<template #thumbnailPlaceholder> | ||
<div class="default-folder-icon"> | ||
<KIcon |
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.
I see. I will try and fix it asap
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.
@marcellamaki @AllanOXDi for context, the placeholder element, here icon, is not displayed only when there is no thumbnail image but also while it's loading. This is a progressive loading experience, important particularly on slower networks.
The bug here is caused by KCard
not detecting that the image has finished loading. I recently fixed this and it will be available in my upcoming KDS PR.
However @AllanOXDi on Kolibri side, it may be helpful to double-check how the icon looks like on smaller resolutions when the thumbnail image is not available. It shouldn't overflow the area in this way and Kolibri needs to configure this.
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.
@AllanOXDi so you can just fix the icon placement and size here if needed, but please don't add any logic regarding whether it's present or no.
Summary
The PR introduces a new folder card to Kolibri to support of coach resource discovery . See #12318
Screen.Recording.2024-07-25.at.19.53.42.mov
closes #12318
References
#12318
Reviewer guidance
While reviewing this PR, please note that I have decided to leave out a few acceptance criteria requirements, namely support for RTL.
Please note that this was built on top of the KCard, which supports two layouts (i.e., vertical and horizontal) with their respective thumbnail display options and for folder cards, the thumbnail display area is currently shown on the left when the display is large. but it require it to be on the right when the thumbnailDisplay prop is set to 'large', in addition to the title prop taking up the position of the above title slot in these kinds of layouts. This is currently not covered by KCard.
Vs the design spec
After discussing with @MisRob, we decided to work with what KCard supports for this issue and file a follow-up issue on KDS to handle these edge cases.
I have made a minor stying adjustment to the below title slot on KCard . To test this PR you would need to work with this KDS PR learningequality/kolibri-design-system#688
Here are a few things to look at:
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)