Refactor hybrid learning card grid #9063

marcellamaki · 2022-01-28T16:36:11Z

Summary

Removes HybridLearningCardGrid
Fixes #9004
Fixes #8983

HybridLearningCardGrid was clearly doing too much, and not doing any of it especially well. I approached refactoring by "unabstracting" to hopefully make things simpler.

For the Library and Topics pages, which continued to have very similar layouts and variation only in the number of columns, I created a new component called LibraryAndChannelBrowserMainContent with just the elements from HybridLearningCardGrid that were needed for these two pages. This new component uses the existing CardGrid component for grid displays as needed. It also has associated tests 🎉
(Separately, I've gotten extremely reasonable/helpful feedback from @sairina that the mismatch between channel cards and TopicsPage is confusing. This is a first step in moving towards more consistent naming.)

For the Bookmarks page and lessons playlist page, there was no need for grids at all, as the content is displayed in "list" style cards. The relevant cards were added directly to their pages.

This approach I think remedies my previous poor decision of HybridLearningCardGrid having to keep track of which page it was on, to know what to display, and overall makes the card displays less brittle. It also reduces the number of levels which props and events need to traverse on some pages, which is nice! I do not think this makes the code any shorter, but hopefully this is a step in the right direction towards readability and maintainability.

Suggestions for other improvements, and whether they are blocking or follow-up, would be welcome and helpful!

…

Reviewer guidance

Because this refactor touched a lot of the learn pages, I have done my best to layout what would need to be reviewed for manual QA.

Lesson page
(Home Page > Your Classes > Your lessons > [Lesson]

list of lesson resources should display
clicking on the lesson should open the resource
using the "back" arrow within the content renderer should return the user to the lesson page

Library Page

list of resent resources should display when there are recent resources
list of search results should display when there are search results
users should be able to toggle between list and grid view
users should be able to open the info panel with the "i" button on each card
users should be able to open the copies modal on resources that have multiple copies
clicking on the content should open the resource
using the "back" arrow within the content renderer should return the user to the library page

Topics/Channel Browsing Page

list of folders should display when there are folders
list of content items within a folder should display when at that level within a folder
list of search results should display when there are search results
users should be able to open the info panel with the "i" button on each card
clicking on the content should open the resource
using the "back" arrow within the content renderer should return the user to the topics page

Bookmarks page

list of bookmarks should display when there are bookmarks
users should be able to open the info panel with the "i" button on each card
users should be able to remove the bookmark with the "x" button on each card
removing the bookmark should remove the bookmark from it's "current place in line"
clicking on the content should open the resource
using the "back" arrow within the content renderer should return the user to the bookmarks page

Overall

for library and topic/channel pages, the grid should update the number of columns on resize to medium and small screen widths
for all mobile devices, a single column of cards should display (never that weird bug with two columns of very narrow skinny cards that was popping up)

tl;dr --manual QA should elicit a response somewhat like "Why has Marcella spent several days on this code? nothing is different??"

Testing checklist

Contributor has fully tested the PR manually
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

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
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 (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

radinamatic · 2022-01-31T22:55:13Z

@pcenov Let's give this a walk-through with built assets (EXE, DEB & APK), thank you!

pcenov · 2022-02-01T11:58:58Z

Tested with the Buildkite assets in Windows 10 (Chrome, Firefox, Edge, Internet Explorer), Ubuntu (Chrome, Firefox) and Android (the app and Chrome) as well as in Safari browser on iPad Air simulator. Mostly everything besides the Search and the 'i' icon in IE is fully functional, I was able to identify only the following issues:

Issue	Screenshot
Library Page - Search by keyword or filter returns no results if the user is not signed in or hasn't viewed any resources yet
:-------------------------:	:-------------------------:
Library Page - Search returns incorrect number of results and View more is not functioning (an extended version of the first issue, the Recent items are displayed as search results) - Steps to replicate: 1. Create a new user. 2. Go to Library and open a resource so that there's something in the Recent section 3. Filter by Language: English
:-------------------------:	:-------------------------:
Library Page - Search with applied filter returns unfiltered results
:-------------------------:	:-------------------------:
Internet Explorer Browser only - User is not able to open the info panel with the "i" button on each card

marcellamaki · 2022-02-01T13:59:26Z

Thanks so much @pcenov -- will address these today!

marcellamaki · 2022-02-01T15:45:36Z

@pcenov -- the first three errors seemed to all be due to me accidentally passing a wrong piece of data, so I think I've solved that and will update shortly 🎉

For

Internet Explorer Browser only - User is not able to open the info panel with the "i" button on each card

one question: is this issue present on all pages, or is it just the bookmarks page? i.e. is it broken on the Library page too?

Thank you!

pcenov · 2022-02-01T15:51:27Z

one question: is this issue present on all pages, or is it just the bookmarks page? i.e. is it broken on the Library page too?

Yup, it's persistent on all pages.

marcellamaki · 2022-02-01T21:31:32Z

Hi @pcenov and @radinamatic -- I've pushed the changes to fix the first 3 issues. I tested the fourth issue in a virtual IE browser and was able to replicate it. I checked out the current version of 0.15.x and it seems to exist there too. So, @pcenov I think you might have found an existing bug. At first I thought perhaps it was from the card refactor Devon and I worked on that was just recently merged, but it seems to be existing on both the bookmarks cards (which were refactored) and the library page (which was not).... At some point, could one of you double check to see if this is a problem on 0.15.x? I want to make sure, just since I am testing this not on an actual windows machine with IE. But, with my virtual browser, I am getting some weirdness with buttons in general (see below -- I clicked on each button that I hover over, but none of those clicks caused anything to happen 🤪), so I'm not sure quite what is going on.

It's not urgent, since this won't be merged right away.

nucleogenesis

Things look good here to me. Just a note on a way to make the tests a bit more robust. Let me know if you run into issues with it.each if you end up taking that route.

kolibri/plugins/learn/assets/src/views/LibraryAndChannelBrowserMainContent.vue

nucleogenesis · 2022-02-01T23:14:18Z

kolibri/plugins/learn/assets/test/views/library-and-channel-browser-main-content-grid.spec.js

+        expect(wrapper.find('[data-test="non-mobile-card-grid"]').element).toBeTruthy();
+        expect(wrapper.find('[data-test="content-card"]').element).toBeTruthy();
+      });
+      it('does not display a `CardGrid` with a `ResourceCard` for each content node in a single, full-width column', () => {


This and some of the other tests like it doesn't test that "each content node" is rendered rather it only tests that the one in your propsData.contents does. Testing two would better cover the condition and wouldn't be too hard to do manually - you could try making each card's data-test dynamically generated :data-test="'content-card-' + idx" and then assert that they both show up. If you want to test more than two to be even safer, then it.each might help.

@nucleogenesis -- thanks for the help here. I've updated the tests now that we're ready to merge into develop again. Let me know if they are similar to what you were thinking. Thanks!

Yup this is great! Thanks Marcella!

pcenov · 2022-02-02T13:44:29Z

Hi @marcellamaki retested with the latest builds on Windows, Ubuntu and Android and I confirm that the search issues are now fixed and there are no other issues. Awesome!
Installed the official 0.15 build and the info icon is working correctly in Internet Explorer so there must be an interim change that's caused this issue there which I guess I can report separately?

…Library Page

… since no grid is used on the page

…r and replaces the HybridLearningCardGrid for these pages

…ince no grid is used on the page

… there is a single card

nucleogenesis · 2022-02-28T19:38:59Z

Does this need any further testing review @marcellamaki or did you re-request my review re: the tests only?

marcellamaki · 2022-03-01T15:18:43Z

Just for the tests! Thanks @nucleogenesis !

marcellamaki marked this pull request as draft January 28, 2022 16:36

marcellamaki force-pushed the refactor-hybrid-learning-card-grid branch from 9fd63b6 to a0038fd Compare January 31, 2022 15:03

marcellamaki marked this pull request as ready for review January 31, 2022 15:18

marcellamaki changed the title ~~Refactor hybrid learning card grid~~ [WAIT TO MERGE] Refactor hybrid learning card grid Jan 31, 2022

marcellamaki requested review from nucleogenesis, MisRob and radinamatic January 31, 2022 20:37

nucleogenesis approved these changes Feb 1, 2022

View reviewed changes

pcenov mentioned this pull request Feb 11, 2022

Internet Explorer - The info side panel is not displayed #9100

Closed

marcellamaki added 8 commits February 21, 2022 15:34

Replace HybridLearningCardGrid with relevant code from component, on …

82a8344

…Library Page

Replace HybridLearningCardGrid with just the ListView card component,…

762f946

… since no grid is used on the page

Create LibraryAndChannelBrowserMainContent component, which is simple…

d0a1869

…r and replaces the HybridLearningCardGrid for these pages

Replace HybridLearningCardGrid with just the lesson card component, s…

34cebe6

…ince no grid is used on the page

Add tests for new LibraryAndChannelBrowserMainContent component

6281ce3

switch to 'let wrapper' and 'beforeEach()'

4fabf61

Fix search results display error, replacing reference with

ee407d8

Update tests to use it.each, thus testing multiple cards, not just if…

83dfbaf

… there is a single card

marcellamaki force-pushed the refactor-hybrid-learning-card-grid branch from 6fa3db3 to 83dfbaf Compare February 22, 2022 16:21

marcellamaki changed the title ~~[WAIT TO MERGE] Refactor hybrid learning card grid~~ Refactor hybrid learning card grid Feb 22, 2022

marcellamaki requested a review from nucleogenesis February 22, 2022 16:22

marcellamaki merged commit 1fd1fc0 into learningequality:develop Mar 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor hybrid learning card grid #9063

Refactor hybrid learning card grid #9063

marcellamaki commented Jan 28, 2022

radinamatic commented Jan 31, 2022

pcenov commented Feb 1, 2022

marcellamaki commented Feb 1, 2022

marcellamaki commented Feb 1, 2022

pcenov commented Feb 1, 2022

marcellamaki commented Feb 1, 2022

nucleogenesis left a comment

nucleogenesis Feb 1, 2022 •

edited

Loading

marcellamaki Feb 22, 2022

nucleogenesis Feb 28, 2022

pcenov commented Feb 2, 2022

nucleogenesis commented Feb 28, 2022

marcellamaki commented Mar 1, 2022

Refactor hybrid learning card grid #9063

Refactor hybrid learning card grid #9063

Conversation

marcellamaki commented Jan 28, 2022

Summary

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

radinamatic commented Jan 31, 2022

pcenov commented Feb 1, 2022

marcellamaki commented Feb 1, 2022

marcellamaki commented Feb 1, 2022

pcenov commented Feb 1, 2022

marcellamaki commented Feb 1, 2022

nucleogenesis left a comment

Choose a reason for hiding this comment

nucleogenesis Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

marcellamaki Feb 22, 2022

Choose a reason for hiding this comment

nucleogenesis Feb 28, 2022

Choose a reason for hiding this comment

pcenov commented Feb 2, 2022

nucleogenesis commented Feb 28, 2022

marcellamaki commented Mar 1, 2022

nucleogenesis Feb 1, 2022 •

edited

Loading