Refactor bookmarks cards #8461
Refactor bookmarks cards #8461
Conversation
There was a problem hiding this comment.
Will review the code, but first two quick design questions - cc @jtamiace
Icon buttons
It's rare in Kolibri (versus Studio) to use purely icon buttons rather than text-based so I wanted to make sure this was intentional. This is partly because learners on tablets don't have tooltips, and partly because learners are less likely to know iconography conventions in contrast to Studio where we use icon buttons more liberally for "power users".
| deletion in bookmarks | deletion in coach UI |
|---|---|
 |
 |
If we do stick with icon buttons, note that we might be using the wrong one for 'delete':
- https://design-system.learningequality.org/icons/#token-clear
- https://design-system.learningequality.org/icons/#token-delete
Empty states
We've made 'empty states' more consistent, and I think this design deviates slightly from previous directions:
- Add empty states to various coach tables and widgets #5051
- consistency updates to empty tables #3550
I would have expected it to look more like:
Perhaps something to track in KDS?
|
We should probably go with your suggestion for the empty state. I'm not too worried about the icon button for removing the bookmark because it's used in the Learn interface for closing out of content and quiz renderers, so hopefully already an association with "making this go away." There's also a way to confirm what it does when they interact with it, and an undo action that if it was unintentional.
|
| }, | ||
| }, | ||
| created() { | ||
| ContentNodeResource.fetchBookmarks({ params: { limit: 25 } }).then(data => { | ||
| this.more = data.more; | ||
| this.bookmarks = data.results; | ||
| this.bookmarks = data.results.map(normalizeContentNode); |
There was a problem hiding this comment.
Although it's possible that API returns an empty array in data.results and therefore map won't fail even when there are no bookmarks, I'd suggest adding a guard to check if data.results are truthy before trying to use map just in case it was undefined or null, just to be sure. Something like
this.bookmarks = data.results ? data.results.map(normalizeContentNode) : []
| text-align: center; | ||
| } | ||
|
|
||
| .bookmarks-header { |
There was a problem hiding this comment.
I am not sure how this would look like here on this page and am using this style only as a good example so @marcellamaki no changes may be needed here if results are not acceptable at this point.
Generally speaking, I'd rather use h1 as is without this style even though it might mean that it won't be pixel perfect, and then eventually update our global styles for fonts, maybe even using a system based on rem units. For this reason, I've been trying to avoid updating font styles in hybrid learning work unless it looked really necessary. I am afraid that otherwise, this approach would lead to styling titles and texts on each page separately and will tend to end up with many inconsistencies across the whole app.
My suggestion would be to ignore tweaking fonts altogether (unless something looks very horrible without doing so) and having a GitHub issue for texts styles. I am not sure though about how our current system works and if changes to it could lead to problems in parts of the app that don't have this new hybrid learning look so we could also use that issue to think through this.
@indirectlylit @jtamiace any thoughts?
There was a problem hiding this comment.
I agree - didn't realize the header was a different style than other headers in the app. Plain h1 would be better for consistency, and I don't see a reason why the header on this page needs to be different than the others.
There was a problem hiding this comment.
+1 for universal styles in typography. I feel like this should live in KDS both as guidance and as some sort of CSS or SCSS mixins that we can use to define consistent cross-product styles.
|
Are the left and right margins on one of your screenshots intentional? Designs have the same left and right margin as the top margin (24px): https://www.figma.com/file/uiZuCIqh2KYvBUfbCiJyyA/Hybrid-Learning?node-id=130%3A11752 |
I'd say that no one can say for sure so I'd suggest checking all places where If there are some regressions, we'll need to have a look if that particular part of the app is going to be refactored into hybrid learning overcoat before the next release or no and based on that, decide if we need to adjust
I'd say this is the same question. It'd be ideal to use (and adjust if needed) our new thumbnails that were made for hybrid learning in general ( |
| :description="content.description" | ||
| activityLength="shortActivity" |
There was a problem hiding this comment.
Is this a temporary value?
Regarding activity length, there is some logic prepared in LearningActivityLabel . So far it's been used to show duration or its textual representation on the home page. If it'd be useful, feel free to move some of this code from the component somewhere higher in the structure and use it for bookmarks.
| return this.createdDate; | ||
| }, | ||
| bookmarkCreated() { | ||
| const time = this.$formatRelative(this.ceilingDate, { now: this.now }); |
There was a problem hiding this comment.
Will the time be correct if I have a page with this component open for a couple of minutes and then bookmark something? I haven't tested this myself yet though from what I see in code my guess would be that right now this.now would rather return time when this component was mounted and not necessarily when a bookmark was really created.
There was a problem hiding this comment.
This card component is used for the bookmarks page itself, and so I am trying to figure out a way where it might be mounted and then have a bookmark created after without a re-navigation. I have tried testing this and so far it seems that the bookmarks time is reflecting accurately. But if you have a specific scenario in mind (or if someone else has one!), I would be happy to give it a try!
There was a problem hiding this comment.
Thank you for checking it. From your explanation, it seems that it creates a hidden dependency from where this component is being used so even though things work for the bookmarks page, I'd recommend using something like this removed implementation so that we can be sure that it will work as expected no matter of scenario since this is a general component. Related to that, ContentCardListViewItem is used in ContentCardGroupGrid which seems to be imported into another four pages.
There was a problem hiding this comment.
Oh, now I see what you're saying. I will re-add the code for it to work as expected, and I also have in my note today to follow up on the usage of ContentCardListViewItem and the other places ContentCardGroupGrid is used, because I noticed that yesterday when I was working on something else. Thanks Misha!
There was a problem hiding this comment.
Sure, and regarding those components being used from both non-hybrid-learning and hybrid-learning, I hope we can find something simple to not block this PR way too much :) Just from the code, those various dependencies look to be "meeeh" :)) Let me know please if I can help with that somehow.
| describe('When the user clicks the remove from bookmarks icon', () => { | ||
| it('will make a call to remove the bookmark from the list of bookmarks', () => { | ||
| wrapper.findComponent({ name: 'ContentCardGroupGrid' }).vm.$emit('removeFromBookmarks'); | ||
| expect(removeFromBookmarksSpy).toHaveBeenCalled(); |
There was a problem hiding this comment.
This doesn't test if a call was made. If there was an error in removeFromBookmarks method, this test would still pass even though the call wasn't made. That's one of the reasons why it's usually not recommended to test internal methods of components.
I'd recommend mocking client and checking that it's been called with correct method and url parameters.
| }); | ||
| it('clicking the load more button calls the load more function', () => { | ||
| wrapper.find("[data-test='load-more-button']").vm.$emit('click'); | ||
| expect(loadMoreSpy).toHaveBeenCalled(); |
There was a problem hiding this comment.
Similar to the above, checking that some method inside of a component has been called doesn't say much about if things actually work. I'd recommend checking that fetchBookmarks on ContentNodeResource has been called with correct params.
|
@jtamiace - I am wondering about how to proceed with the thumbnails and card spacing from a design point of view. I'll include a few screenshots below, and please let me know if you have questions. With the thumbnails, since we have a variety of image ratios, there are some constraints that are used to contain the image (i.e., it shrinks to fit its boundaries, rather than being cropped). This is less noticeable with the list card view, but the top margin of the card is offset, see below:
(full image container background - and alignment - visible in grey)
But by keeping the width fixed, the difference in height becomes more noticeable on mobile especially. I can fill the background of the image's container as a light grey (which is something @MisRob has implemented with her new thumbnails work, although I don't think the component will be appropriate to use here but maybe can be added with a larger card refactor) which to me seems to keep the margin feel consistent, since there is a clear separation between "bonus space" of the thumbnail and the white of the margin. It also feels less "crisp" (I can't think of a better word - hopefully you will know what I am trying to get at)... For example, in some various mobile views:
So, I guess the question is, which option would you prefer that I use? Also, if @MisRob or @indirectlylit have alternative suggestions of how I might manage this, would be very open to hearing them! |
|
Regarding the thumbnails, as I mentioned above I think it's completely fine to not use the new |
There was a problem hiding this comment.
One additional item to note is that there is some usable whitespace in the mobile cards between the footer & the text. I think maybe an 8px vertical padding on the activity type (the icon & "watch/read/etc" bit) in mobile might give some good spacing between the thumbnail and the activity and the title.
Of course, items with small descriptions or no description are doomed to have some whitespace there, but this example is truncated. Maybe even using the standard font size could make use of the available space naturally without needing.
I quickly tried it out in devtools and a combination of these changes feel to maximize the use of available whitespace (but would need some testing, maybe tweaking for desktop vs mobile).
.metadata-info {
margin: 8px 0; // This won't look right on desktop though
}
.text {
/* other .text styles */
font-size: 16px;
}
| .chip-right { | ||
| position: absolute; | ||
| right: 10px; | ||
| bottom: 20px; | ||
| height: 34px; | ||
| padding: 8px; | ||
| font-size: 13px; | ||
| background-color: rgba(0, 0, 0, 0.7); | ||
| border-radius: 4px; | ||
| } | ||
|
|
||
| .chip-left { | ||
| position: absolute; | ||
| bottom: 20px; | ||
| left: 10px; | ||
| height: 34px; | ||
| padding: 8px; | ||
| font-size: 13px; | ||
| background-color: rgba(0, 0, 0, 0.7); | ||
| border-radius: 4px; | ||
| } |
There was a problem hiding this comment.
You don't have to account for both LTR and RTL with CSS classes in this case. We have some magic in our build system that automatically creates those styles. @indirectlylit (I think) may be best to describe how this works.
I just tested it - if you just create one class and style for LTR like so:
.chip {
left: 10px;
/* the rest of the styles */
}
Then change your language to Arabic and check the CSS on that chip class and you'll see that the left in your code is a right in the browser.
There was a problem hiding this comment.
Separately, and this may be something to discuss in KDS as well, I received early on and have since held closely to the guidance that we should try to work within 8px increments wherever possible, 4 if needed, which is derived to some extent by Material guidelines.
I double checked KDS and see that we don't really have such guidelines, although it would be nice to have some. In any case, it's fairly nitpicky and looks fine as-is in the browser. Would love @indirectlylit and @MisRob 's thoughts w/ regard to whether we should aim to codify some aspect of this guidance in KDS or if it should just be a Kolibri thing (or if we are getting little/no benefit from the long-held norm).
I personally appreciate consistency in the code and have found trying to do everything at 8/16/24/etc px has been pretty painless in most cases.
There was a problem hiding this comment.
One last note here is that I think that the height entry is unnecessary, you're expanding the container vertically with padding already and I didn't see any change when I removed it locally.
There was a problem hiding this comment.
Separately, and this may be something to discuss in KDS as well, I received early on and have since held closely to the guidance that we should try to work within 8px increments wherever possible, 4 if needed, which is derived to some extent by Material guidelines.
I double checked KDS and see that we don't really have such guidelines, although it would be nice to have some. In any case, it's fairly nitpicky and looks fine as-is in the browser. Would love @indirectlylit and @MisRob 's thoughts w/ regard to whether we should aim to codify some aspect of this guidance in KDS or if it should just be a Kolibri thing (or if we are getting little/no benefit from the long-held norm).
I personally appreciate consistency in the code and have found trying to do everything at 8/16/24/etc px has been pretty painless in most cases.
Sorry, just saw this message!
Thanks for flagging this @nucleogenesis. This is in fact already documented in the design system here:
https://design-system.learningequality.org/layout/#spacing
For more context, folks can read about "baseline grids" in the context of Material Design or in general.
| .footer-right { | ||
| display: block; | ||
| float: left; | ||
| } |
There was a problem hiding this comment.
These icons ought to be flipped to the other side but the RTL version puts the icons under the thumbnail.
I'd suggest using flex here instead of floats. I think that floats aren't really supposed to be used for layout and that their primary purpose is to allow elements to flow around something that is stuck to one side.
I think that something like this could work if you remove the footer-left/right styles and classes altogether. I tried it out quickly and seems to work well, but the footerIcons prop will need to be ordered so that the X is the outer-most icon. The flex-end automatically aligns everything to the correct side between RTL/LTR. Then the changes to metadata-info-footer align everything vertically when in mobile.
.footer {
position: absolute;
bottom:0;
display: flex;
justify-content: flex-end;
width: 100%;
padding: 16px;
}
.metadata-info-footer {
flex: auto;
align-self: center;
margin: 0;
font-size: 13px;
}
|
Updated notes on changes for devs
Updated QA guidance for @radinamatic and @pcenov (and @jtamiace for design, if so inclined) Core functionality to test:
While there are a few adjustments that still need to be made to the cards, most of the card UI is "done" and UI/design review can include these points:
NOTES: Progress/Completion bug weirdness is popping up here too, so the progress bar may or may not be visible, based on what you are looking at! My recommendation right now is to put a pin in that part of the testing 📌 Known TO-DOs are pointed out in the screenshot below (learning activity icon and string is missing and the placeholder is messing up some spacing, channel icon and channel title for alt text missing, activity length is a placeholder 'short activity') |
|
Hi @marcellamaki and @radinamatic Here is the test report:
|
marcellamaki
force-pushed
the
refactor-bookmarks-cards
branch
from
953d83b
to
25d181b
Compare
|
Hi @pcenov - thank you for the QA given the challenges of known bugs. I've also realized one last code change I made to fix the icon I didn't push to github, I just had it locally, so I apologize for the oversight and for the difficulty you encountered while testing this. Hopefully as Jacob and I push though this last bit of frontend work and integration in the next two weeks, we will soon have much more complete (and less frustrating) features for you to test! |
There was a problem hiding this comment.
- The bug Radina noted in the other PR is not fixed and doesn't feel in scope here (you've not changed the source of the issue). I mentioned this in #hybrid-learning on Slack that I fixed that toggle locally so that I could add bookmarks in order to test being able to delete them properly.
You can see my commit here - it's not fully tested but it let me add bookmarks where I couldn't initially.
Generally, this is coming together great. Looks good, feels good. Just a couple issues outstanding and then should be good to go!
kolibri/plugins/learn/assets/src/views/HybridLearningContentCardListView.vue
Outdated
Show resolved
Hide resolved
marcellamaki
force-pushed
the
refactor-bookmarks-cards
branch
from
25d181b
to
242a561
Compare
…cards, rather than modify existing components, to prevent regressions
…t content bookmark status in LearningActivityBar
…instead of ContentCardGroupGrid
marcellamaki
force-pushed
the
refactor-bookmarks-cards
branch
from
242a561
to
473ead3
Compare
|
@marcellamaki I finally (sorry) just got to get to test this and I'm not seeing any content in channels and the cards in the library page don't do anything so I cannot add any Bookmarks. I tried rebasing the branch locally but still wasn't seeing any content. If this isn't a known issue resolved elsewhere, I'd hold off on merging before the bug bash. |
|
@nucleogenesis - the issue you describe is a result of me doing a sub-par rebase (things to learn for the future!). It is fixed in the subsequent PR for the Library Page. Obviously now we are not hurrying to get in before the bug bash, so no need to merge quickly, but just as an FYI. If you're able to access content from the home page, it should open the content renderer correctly. (It is working locally for me on this branch). If that's a feasible workaround for this PR, and you can review what you need, that's great, but if not, just let me know and I can scoot some of the updates to the Library page back into this PR tomorrow so that it's a bit easier to work with. [ETA: I am just going to pull that code in in the morning so everything is easier to review!] |
|
Superseded by #8548 |
Summary
This PR updates some of UI element conflicts that surfaced during string freeze on the Learner bookmarks page, with the main changes being:
Notes (out of scope)
References
Figma specs
No bookmarks
Desktop view
Note: the channel thumbnail is going to be added as new metadata, so it is not visible yet and you just see the alt text (and relatedly, the channel title is not yet added, so the title is not being added to the alt text as of yet)
Various mobile previews (Chrome dev tools)
…
Reviewer guidance
ContentCard/card.scss- is this going to cause a conflict elsewhere?Testing checklist
PR process
Reviewer checklist
yarnandpip)