Remote browsing filters by rtibbles · Pull Request #10986 · learningequality/kolibri

rtibbles · 2023-07-17T18:19:02Z

Summary

Updates logic for displaying whether categories are enabled or disabled within a channel, after incomplete refactor
Makes SearchFiltersPanel responsible solely for displaying search filters
Makes SearchFiltersPanel handle its own modal display when needed
Removes duplicated section tag
Updates SidePanelModal to remove Search filters panel specific aria label, and insert via a prop instead
Create TopicsPanelModal for displaying topics in the TopicsPage
Moves all 'currentCategory' logic inside SearchFiltersPanel to encapsulate and reuse logic
Uses vue2-teleport to allow KModal use from within a fixed position div, which seems to have been the reason it was previously embedded outside of the SidePanelModal component.
Update how we display selected categories in the category selection modal to match the original spec
Update how category selection works, so that all actual categories (but not 'Uncategorized') are additive in their selection, meaning:
- Selecting a category when another category is already selected will now filter by both categories
- Selecting a category when that category is already selected will now remove filtering by that category
- Selecting a category when any of its descendant categories are selected will deselect the descendant categories, and only select the ancestor category that the user clicked
Fix bug in metadata label filtering, whereby it was doing an OR when multiple categories, learning activities, etc. were selected rather than an AND
Adds a regression test for this bug

References

Fixes #10975
Fixes #10976
Implements this design:

Reviewer guidance

Removing the aria-label now means that the SidePanelModal doesn't have an aria-label in other places where it is used, but as the previous label was incorrect, this doesn't seem like a regression. We should open a follow up issue for this.

I am very sure that none of the code moves I did moved any strings around, but a double check would never hurt.

I may not have implemented the above design precisely to spec, as the highlight is the same width as the text, not the overall block - so that may need an update? Could also be filed as a follow up issue.

For manual testing, this needs to be tested for searching and filtering on the Library page (both local and remote for peers) and within channels (both for local and remote peers).

I am very sure that the bug that is the root cause of the OR behaviour noted in #10975 is present in Studio, even if it is not manifesting (probably because of differences in Library content).

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

Move topic listing to separate component. Remove duplication of <section> and search components.

…containers.

Change category selection to be cumulative.

github-actions · 2023-07-17T18:50:37Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0a20.dev0_git.32.g7e064224.pex
Windows Installer (EXE)	kolibri-0.16.0a20.dev0+git.32.g7e064224-unsigned.exe
Debian Package	kolibri_0.16.0a20.dev0+git.32.g7e064224-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0a20.dev0+git.32.g7e064224-0.3.0.dmg
TAR file	kolibri-0.16.0a20.dev0+git.32.g7e064224.tar.gz
WHL file	kolibri-0.16.0a20.dev0+git.32.g7e064224-py2.py3-none-any.whl

pcenov · 2023-07-18T16:15:36Z

Hi @rtibbles,

The issue described in Kolibri Library - It's possible to filter by more than one activity type #10975 is still present - does that mean that it has to be reported and fixed on Studio's side? Perhaps it's worth discussing and specifying the expected behavior for the Categories and Activities filters as it's different from the rest (we are adding up results and not excluding). That becomes especially obvious when using these filters together with a Keyword for example in which case only results containing the keyword are being returned, the categories in the modal are displayed as disabled and cannot be deselected:

2023-07-18_19-08-33.mp4

2 Observed a minor issue where the selected descendant categories are not being highlighted:

1.Subcategory.selection.mp4

I've reported separately an issue about the Language filter: Explore libraries - The applied language filter and the 'Clear all' option are missing #10989

rtibbles · 2023-07-18T16:18:32Z

For 1) as I mentioned above:

I am very sure that the bug that is the root cause of the OR behaviour noted in #10975 is present in Studio, even if it is not manifesting (probably because of differences in Library content).

So, this should be functioning correctly now for everything except Studio, and we can port the fix for that to Studio as well.

2 is what I was meant to be implementing here, so I'll fix that within the scope of this PR.

I've self assigned 3, but we can deal with it separately.

rtibbles · 2023-07-18T16:20:55Z

Perhaps it's worth discussing and specifying the expected behavior for the Categories and Activities filters as it's different from the rest (we are adding up results and not excluding).

This was never intended behaviour, it just hadn't manifested previously because a) we didn't have multiple learning activities for individual resources, and b) category selection before this PR cleared previous category selections.

I think maintaining consistent 'exclusion' filtering - so a resource needs every applied filter to appear seems like the right way to go for now. We can potentially add interactions to allow more advanced OR filtering in the future. This is how it should be functioning for local filtering and filtering on a peer running this code now, just remains for the port to Studio to have this happen universally.

…led or highlighted.

Align styling closer to original spec.

rtibbles · 2023-07-18T18:22:28Z

OK, the bug should be fixed, and in collaboration with @jtamiace I have attempted to update the category modal interface closer to the original spec - removing sub category indentation and using bolding instead:

marcellamaki · 2023-07-18T23:54:10Z

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

  import NotificationsRoot from 'kolibri.coreVue.components.NotificationsRoot';
-  import { PageNames } from '../constants';
  import plugin_data from 'plugin_data';
+  import { PageNames } from '../constants';


is this just from linting?

Yes, for reasons I couldn't discern, linting changed its mind while I was making this PR and decided to move things around.

pcenov

Implemented as specified and looks great! No new issues observed.

radinamatic

Filtering now looks much better, good to go! 🎉 💯

marcellamaki

I think this is good to merge! I added one comment re: button styling but it's for the designers. Thanks for all the cleanup here.

marcellamaki · 2023-07-19T14:47:02Z

kolibri/plugins/learn/assets/src/views/SearchFiltersPanel/index.vue


-  <section
+  <component
+    :is="windowIsLarge ? 'section' : 'SidePanelModal'"


I like this. It is probably something we (or at least I )can make use of more often

Yeah, the alternative pattern that was being used was for the main body of the component to be in a child component and then use a v-if on the different potential parent components.

This method is a little more concise, but does mean that for the section tag it has a bunch of random meaningless attributes that should only be passed to the SidePanelModal component.

marcellamaki · 2023-07-19T15:20:57Z

...gins/learn/assets/src/views/SearchFiltersPanel/CategorySearchModal/CategorySearchOptions.vue

+          borderStyle: 'solid',
+          borderRadius: '4px',
+        };
+        const appearanceOverrides = {


This is a more general thought, but there are quite a few places in the hybrid learning work where we are doing a pretty "unique" variation on our KButton that requires a lot of overrides. Might be worth considering if we want a core button style in the KButton upgrades to cover this when we do the updates for that component, or if we want to just continue this way as needed and essenitally swap the colors @jtamiace @tomiwaoLE

I think it might be worth revisiting the API of KButton at a minimum to see if there's a more natural way to apply this kind of styling.

marcellamaki · 2023-07-19T15:22:11Z

kolibri/plugins/learn/assets/src/views/TopicsPage/ToggleHeaderTabs.vue

        // navigates the main page to the search view
        if (this.topic) {
          const query = { ...this.$route.query };
-          delete query.dropdown;


I have no recollection as to why I even added this in the first place!

It was because we used to have a query parameter that determined whether the side panel state was toggled open, I think? Then we refactored it to rely on component JS state, but this was never cleaned up.

marcellamaki

I think this is good to merge! I added one comment re: button styling but it's for the designers. Thanks for all the cleanup here.

rtibbles added 5 commits July 17, 2023 11:04

Fix enabling/disabling of categories.

82fffe8

Consolidate search selection behaviour in search filters panel.

8684633

Move topic listing to separate component. Remove duplication of <section> and search components.

Use vue2-teleport to avoid issues with embedded KModals inside fixed …

d94917f

…containers.

Update category selection interface to spec.

4fd45a8

Change category selection to be cumulative.

Update bitmask comparison to AND rather than OR.

f832ef0

rtibbles added this to the Kolibri 0.16 Release: Bugs and Regressions milestone Jul 17, 2023

rtibbles requested review from marcellamaki, pcenov and radinamatic July 17, 2023 18:19

github-actions bot added DEV: backend Python, databases, networking, filesystem... APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: large labels Jul 17, 2023

rtibbles added 2 commits July 17, 2023 15:55

Fix tests for updated search panel.

23cc6a3

Update linting.

4559bc0

pcenov mentioned this pull request Jul 18, 2023

Explore libraries - The applied language filter and the 'Clear all' option are missing #10989

Closed

rtibbles assigned marcellamaki Jul 18, 2023

rtibbles added 2 commits July 18, 2023 10:50

Fix bug where the lowest level categories would not be properly disab…

01aa4a2

…led or highlighted.

Improve styling of categories, add hover styling.

8b5e993

Align styling closer to original spec.

marcellamaki reviewed Jul 18, 2023

View reviewed changes

pcenov approved these changes Jul 19, 2023

View reviewed changes

radinamatic approved these changes Jul 19, 2023

View reviewed changes

marcellamaki approved these changes Jul 19, 2023

View reviewed changes

marcellamaki reviewed Jul 19, 2023

View reviewed changes

marcellamaki approved these changes Jul 19, 2023

View reviewed changes

marcellamaki merged commit 1ea6137 into learningequality:develop Jul 19, 2023

rtibbles deleted the remote_browsing_filters branch July 19, 2023 17:06

rtibbles mentioned this pull request Jul 28, 2023

updates to visual styling of category selection UI #8787

Closed

Conversation

rtibbles commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

Uh oh!

github-actions bot commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pcenov commented Jul 18, 2023

Uh oh!

rtibbles commented Jul 18, 2023

Uh oh!

rtibbles commented Jul 18, 2023

Uh oh!

rtibbles commented Jul 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcenov left a comment

Choose a reason for hiding this comment

Uh oh!

radinamatic left a comment

Choose a reason for hiding this comment

Uh oh!

marcellamaki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcellamaki left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rtibbles commented Jul 17, 2023 •

edited

Loading

github-actions bot commented Jul 17, 2023 •

edited

Loading