learningequality
Resource discovery: Implement folder and resource card
Summary
The PR introduces a new folder card to Kolibri to support of coach resource discovery . See #12318
https://github.com/user-attachments/assets/cba941f7-55c0-46f9-a524-e8d103e7ed17
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 https://github.com/learningequality/kolibri-design-system/pull/688
Here are a few things to look at:
- Does it have Folder titleF, icon , label and Folder thumbnail picture?
- Folder thumbnail placeholder when a picture is not available?
- Is it responsive ?
Testing checklist
- [x] Contributor has fully tested the PR manually
- [x] If there are any front-end changes, before/after screenshots are included
- [ ] Critical user journeys are covered by Gherkin stories
- [ ] Critical and brittle code paths are covered by unit tests
PR process
- [x] PR has the correct target branch and milestone
- [ ] PR has 'needs review' or 'work-in-progress' label
- [x] If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
- [ ] If this is an important user-facing change, PR or related issue has a 'changelog' label
- [ ] If this includes an internal dependency change, a link to the diff is provided
Reviewer checklist
- Automated test coverage is satisfactory
- PR is fully functional
- PR has been tested for accessibility regressions
- External dependency files were updated if necessary (
yarnandpip) - Documentation is updated
- Contributor is in AUTHORS.md
Build Artifacts
| Asset type | Download link |
|---|---|
| PEX file | kolibri-0.18.0.dev0_git.20240924172741.pex |
| Windows Installer (EXE) | kolibri-0.18.0.dev0+git.20240924172741-windows-setup-unsigned.exe |
| Debian Package | kolibri_0.18.0.dev0+git.20240924172741-0ubuntu1_all.deb |
| Mac Installer (DMG) | kolibri-0.18.0.dev0+git.20240924172741.dmg |
| Android Package (APK) | kolibri-0.18.0.dev0+git.20240924172741-0.1.4-debug.apk |
| TAR file | kolibri-0.18.0.dev0+git.20240924172741.tar.gz |
| WHL file | kolibri-0.18.0.dev0+git.20240924172741-py2.py3-none-any.whl |
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@AllanOXDi After all feedback here is resolved, and also https://github.com/learningequality/kolibri-design-system/pull/752 finished and linked here, please ping me here - will do one final read through and then we should be ready to develop merge.
For resource card, it does not show any metadata. Other than that everything should be okay or close to the design.
Thank you for info @AllanOXDi, that makes sense yes.
I will do the final review next week.
Couple of last notes from me @AllanOXDi, then I think we'll be ready for merge :)
Couple of last notes from me @AllanOXDi, then I think we'll be ready for merge :)
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 @click.stop like you did here should do? https://github.com/learningequality/studio/pull/4480/commits/07a4131b3e242e2f8b35b4e574028d024358dc96#diff-a836f27f6ada2a7aa18ebaa5c94993244781588b3be7930037c89960c3d556bdR34-R35
I assume that
@click.stoplike you did here should do? learningequality/studio@07a4131#diff-a836f27f6ada2a7aa18ebaa5c94993244781588b3be7930037c89960c3d556bdR34-R35
This is something that I already tried.
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.
Thank you @AllanOXDi, I will look at it later today if I can spot something :) As KCard went through many updates since, it's possible something works differently compared to the first Studio version.
@AllanOXDi I looked into the issue with @click.stop not working and it's a regression I caused in KCard a couple of weeks ago. Fixed it here https://github.com/learningequality/kolibri-design-system/pull/774. Would you try to link and confirmed it helps?
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!
@AllanOXDi I think the last thing here is just to clean up any code that was used only for QA, and then we can go ahead and merge! Thank you for all the hard work managing this in connection with the KCard updates
@AllanOXDi I'm sorry there's still some changes that shouldn't be here before merge. Would you always preview "Files changed" tab? I'm not particularly concerned about added/removed empty lines etc. but for example the extra condition in the ContentCardList would cause the current cards to not be present at all. I'd also ask you to double-check other files.
I don't think you pushed it then @AllanOXDi or perhaps internet connection troubles? See here https://github.com/learningequality/kolibri/pull/12418/files
. It's really best to preview "Files changed" because than you can exactly what others see.
Not that it'd be wrong in the scope of this PR - it will just make work easier for those to come because it'd break the grid behavior unexpectedly otherwise. Thanks
Okay, nice. I think as soon as checks pass, this is ready. Thanks for following-up @AllanOXDi, I will leave joy of hitting the merge button for you ;)