Skip to content
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

Update library page #8526

Conversation

marcellamaki
Copy link
Member

@marcellamaki marcellamaki commented Oct 22, 2021

Summary

This PR is to follow on frontend WIP work merged for string freeze #8424 as well as @rtibbles work in #8488.

References

Addresses #8304 and #8305

Figma References

User stories/ workflows overviews

Toggling between list and grid

toggle-list-grid

Filtering with multiple selectors

filter-multiple-selectors

Removing one selected filter, and search updating

remove-one-selected-filter

Using the category search modal

category-search-modal

View more

view-more

Responsive views - screen shots

Description View
Level 4 Screen Shot 2021-10-22 at 9 22 12 AM
Level 3 - Grid and Overlay Screen Shot 2021-10-22 at 9 29 03 AM
Level 2 Screen Shot 2021-10-22 at 9 22 30 AM
Level 2 Screen Shot 2021-10-22 at 9 22 24 AM
Mobile - open side panel Screen Shot 2021-10-22 at 9 23 08 AM
Mobile - Category Search Screen Shot 2021-10-22 at 9 23 14 AM

Reviewer guidance

This is a rather extensive PR that builds on several others, so there are a lot of things to look out for.

What is included in this PR/What to test for - Manual/Design QA:

  • Accurate reflection of Figma specs (cc @jtamiace) - although not all metadata will be displayed

    1. updated card designs
    2. Overall layout
    3. Buttons and tooltips
  • Responsive layout working as expected across browsers and screens

  • Filtering and keyword search as implemented by @rtibbles still working 😄 with some additions:

    1. Can search by keyword
    2. Can search by single filter
    3. Can search by multiple filters
    4. Can view more
    5. Can clear all
    6. Can clear filters individually
    7. Side panel accurately disables and/or hides filters and filter groups that are not available on search results

IMPORTANT NOTES AND CAVEATS -- PLEASE READ!

Currently, there is commented-out code that will conditionally render the category search options (LibraryPage line 408). For the purposes of manual QA on the search modal, I have not implemented this, but it would be good to get developer eyes on.

Relatedly, because of this, it is very possible to get "0 results" with the category search, because we are allowing these filters right now (will not in Production) and there is no metadata!

Known To Dos
(that are definitely in scope for this PR, but for the sake of time, I am opening it before I get all of them in)

Priority before end of sprint and bug bash (in order of importance?)

  • Will need to be rebased off of 0.15.x once the Bookmarks PR is merged, and there might be some amount of merge conflict resolution needed
  • "View Info" button at base of card will (hopefully?) be working after rebase that connects @nucleogenesis 's side panel work
  • strings on filter tags need to be ... legible and helpful 😄 (if anyone has a suggestion of how to implement this in a way that will not make me want to tear my hair out, please let me know)
  • KDS Icons swaps in various places - some tabs, buttons, category search modal, etc. Assume if an icon is incorrect that it is a placeholder
  • Channel Icons need to be added to cards (I think this will require a backend change, @rtibbles?)
  • "active filter" button style on side panel - i.e. the same style on hover should also be visible if the button filter is currently selected
  • Remove from search results // don't show this again feature - not implemented (not to say this is not important, but I am not sure if I can feasibly implement this by Tuesday....)

ALSO TO DO:
The topic/channel browse page is not included with this, I will open a separate PR Monday morning with a final polish there (I am about 75% there on updating the implementation, and much of the core functionality and layout is exactly the same - so I am telling myself it should not require extensive review and testing.... 🤔 )


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

…cards, rather than modify existing components, to prevent regressions
…t content bookmark status in LearningActivityBar
…cards, rather than modify existing components, to prevent regressions
…ch cards are used and grid at different sizes
…rking. No translations/string mapping - using ids
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments - only did a quick skim of the code.

@pcenov
Copy link
Member

pcenov commented Oct 25, 2021

Testing notes based on testing the following Windows and Linux builds: https://buildkite.com/learningequality/kolibri-python-package/builds/3927

1. Toggling between list and grid - functions correctly at the Recent section, not implemented at the Search Results as indicated in Figma:
2. Filtering with multiple selectors - didn’t stumble on any issues with that except the strings on the filter tags (a known issue, right):
3. Removing one selected filter, and search updating - tested and works correctly
4. Using the category search modal - the option is not available in the Buildkite builds, please let me know when it is so that I can test it as well
5. View more - works correctly but I would expect to see such an option when I scroll down to the last displayed card
6. Responsive design: all looks fine with the exception of the cards which are not aligned properly and displayed correctly in the search results as well as on the Library page:
7. The drop-down text is not positioned as in Figma:
8. Activities section issues:
9. Deleting the search term filter tag results in incorrectly showing the search field with the delete icon

@marcellamaki
Copy link
Member Author

Thank you @pcenov for your very helpful and thorough QA, as always!

Addressed the following issues:

  1. Toggling list/grid now also appears on search results
  2. View more appears both at the top and at the bottom of list
  3. Layout/alignment updated
  4. Dropdown text fixed
  5. I do see that the "Explore" icon missing (this is a mis-match of naming, and it will be updated in a separate repo tomorrow first). For the other arrow, do you mean that some other icons are missing? They should now be appearing, or at least they are now displaying locally for me. If "create" and "reflect" are not appearing for you let me know, and I will do some more investigation.
  6. @jtamiace Can you let me know if this issue with the search icon is a blocking issue? I think at this stage, the time it would take me for me to change this might be better used elsewhere, and I would be happy to address this in a follow up issue -- but if you'd like to get it resolved now, just let me know, and I will prioritize it.

@jtamiace
Copy link
Member

@marcellamaki I think you're right, the search icon issue can probably wait compared to the other issues.

@pcenov
Copy link
Member

pcenov commented Oct 26, 2021

Great progress made @marcellamaki!

Replying first to your question above 'For the other arrow, do you mean that some other icons are missing?' - I mean that the labels of the icons are positioned exactly in the middle between too icons so that it can be confusing for which one is the label. The design in Figma suggests that the text should be closer to the icon above it.

In addition to verifying the fixed issues I did another round of testing today and here are some additional findings:

  1. Things are not looking great in IE11 - the header is moving all over and there is a horizontal scroll bar displayed for the search results:
2021-10-26_18-28-22.mp4

2021-10-26_18-29-27
2. When there is a small number of search results the cards are displayed squished:

2021-10-26_18-59-00

  1. When there are 0 search results the the View as list/View as grid options should not be displayed at all:

2021-10-26_19-12-23

Some thoughts/suggestions maybe for another PR or just a brainstorming session:

  1. After toggling the View as list/View as grid options the selection should be kept so that the user won't have to toggle them constantly after each search.
  2. The number of results option which is displayed as '25 results View more' is a bit substandard, perhaps this should be rephrased to show the total number of results found and the View more option should be displayed just at the bottom at the page? @radinamatic any thoughts on that?
  3. There's no Clear option in the filter section itself which is not a big deal as there is such option in the results but would probably come handy.

Note that I still haven't had a chance to see the Categories option as it is not displayed in the Buildkite build.

@marcellamaki marcellamaki mentioned this pull request Oct 26, 2021
9 tasks
@marcellamaki
Copy link
Member Author

Superseded by #8548

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants