Integrate search resource selection #13008

AlexVelezLl · 2025-01-15T22:45:52Z

Summary

Integrate search filters panel to lesson resource selection
Add a new immersive header to the side panel modal component.

Compartir.pantalla.-.2025-01-15.17_44_57.mp4

References

Closes #12987

Reviewer guidance

Edit a lesson
Change browser route path from "/lesson" to "lessontemp".
Click on "Manage resources".
Click on the search button from the first page on the side panel, or from within any topic
Play and try any possible combination with different filters, browsing search results topics, browser back/forward buttons navigation, etc.

github-actions · 2025-01-16T15:39:47Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.18.0a0.dev0_git.408.g7a6bd11e.pex
Windows Installer (EXE)	kolibri-0.18.0a0.dev0+git.408.g7a6bd11e-windows-setup-unsigned.exe
Debian Package	kolibri_0.18.0a0.dev0+git.408.g7a6bd11e-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.18.0a0.dev0+git.408.g7a6bd11e.dmg
Android Package (APK)	kolibri-0.18.0a0.dev0+git.408.g7a6bd11e-0.1.4-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.18.0a0.dev0+git.408.g7a6bd11e.zip
TAR file	kolibri-0.18.0a0.dev0+git.408.g7a6bd11e.tar.gz
WHL file	kolibri-0.18.0a0.dev0+git.408.g7a6bd11e-py2.py3-none-any.whl

marcellamaki · 2025-01-16T16:17:29Z

@pcenov - this is not urgent, but depending on how time consuming your laptop reset process is, this could go to the top of your list for Friday or Monday. Thank you!

pcenov · 2025-01-20T14:27:43Z

Hi @AlexVelezLl and @marcellamaki - I confirm that the search is integrated as specified above.

Here are a few general observations based on my testing which can probably be tracked as follow-up issues if not already tracked somewhere else:

We probably need to indicate in a better way within the search panel that there are no channels to import from when there aren't any as currently this is what I see in that case:

As specified in Figma, when we open the search panel the Category filter should be expanded by default while currently it's not:

3 There are quite a few design differences if the current implementation is closely compared to Figma such as the back arrow and the label are not centered, the keywords label should be removed, the icons for the categories are not added etc. Since I am not sure if this is in scope for this PR I am just mentioning it here as a consideration:

When searching by selecting filters it's not immediately clear how the user should proceed the search (the user has to click the search icon next to the empty keywords field). In this case I was expecting to immediately see the filtered results after having applied a filter:

not.clear.how.to.proceed.mp4

When testing the mobile view in Chrome's mobile devices emulator everything looks ok, but when I open the search panel in Chrome on my Samsung Note 10 phone I can only see the 'N resources selected' text and not the "Save & Finish" button as it's covered by the device's taskbar:

nucleogenesis

Overall the changes are looking good. I left a handful of largely non-blocking comments. Happy to chat further about anything I noted.

I haven't finished going through every file yet, so I will be coming back to give a few parts another closer look

nucleogenesis · 2025-01-17T23:59:14Z

kolibri/plugins/coach/assets/src/composables/useResourceSelection.js

+    });
+  };
+
+  const useSearchObject = useBaseSearch({


I think this would be a little easier to read overall if you destructured everything out of useBaseSearch that you're using later on like const { displayingSearchResults, searchTerms, clearSearch, etc } = useBaseSearch(...) rather than assigning the return value to an object.

Looking again on another read, I get the sense that removing the context of where things like results and searchLoading are coming from might make it more confusing down where you use them to define values in searchFetch.

I'm definitely ~~nitpicking~~ pontificating at this point, but putting Object at the end of a variable name is a bit redundant. I get that the idea is to basically say "this is the thing that we got from useBaseSearch" so I think keeping it all as-is would be okay, but generally I try to avoid putting those kinds of things on variable names because anybody reading it can glean that by looking at the variable's definition.

In my experience it sometimes involves more cognitive load for me when we mix these variables that are scoped to an specific context with all other general variables, and more if we are going to use them just a single time, or if we are gonig just to return them from the main composable. But it's just a personal perception 😅, I have destructured the relevant variables from the useBaseSearch composable to see how it looks :), In my opinion, it doesn't hurt and we can keep them destructured, I'm fine with both options :).

For sure - I think in this context it makes sense to name the object and use it when accessing the values so it's clear where they're coming from.

nucleogenesis · 2025-01-18T00:02:12Z

kolibri/plugins/coach/assets/src/composables/useResourceSelection.js

+    loadingMore: useSearchObject.moreLoading,
+    fetchData: async () => {
+      // Make sure that the topic is loaded before searching
+      await waitForTopicLoad();


Calling the function waitForTopicLoad feels redundant w/ the await here when I read it as "await wait for topic load" as opposed to if it were called loadTopic.

Non-blocking as it's perfectly readable as-is and my comment is more a matter of preference

Hmm I have seen these namings in other places and I have taken inspiration from there. e.g. in testing library, waitFor and waitForElementToBeRemoved methods, or this waitFor in this blog.

It makes sense to me that these methods are named like these, because what they do is to wait for something to happen. The await keyword is apart from the intention of the method. And for example, we can just have something like:

waitForTopicLoad().then(() => ...)

and would work exact the same. So in this context I think we can just ignore the await from the action we are expecting, and relate it to the intention of awaiting the promise the method is returning.

Let me know what you think :).

That makes sense to me! Thanks Alex.

nucleogenesis · 2025-01-20T20:53:09Z

...ugins/coach/assets/src/views/lessons/LessonResourceSelectionPage/LessonContentCard/index.vue

    },
    computed: {
      isTopic() {
        return !this.content.isLeaf;
      },
+      headingElement() {
+        return this.headingLevel ? `h${this.headingLevel}` : 'h3';


Non-blocking: given the default value of headingLevel the conditional could be removed here.

...ugins/coach/assets/src/views/lessons/LessonResourceSelectionPage/LessonContentCard/index.vue

AlexVelezLl · 2025-01-21T14:48:18Z

Thanks @pcenov!

I think this can be reported in a different issue :).
Just implemed this!
I have added the "Search in ..." as title on top of the search box when we open the filters from within a foder, and "Keywords" if we open it from the index (where we see the channels and bookmarks).

I think the icons are still not available cc: @marcellamaki, so probably should be reported in a different issue. And the border-bottom in the side panel modal header I think its something we have in develop but not in the figma specs. (honestly I am not sure neither about the reason of this difference but it has been accepted for a long time 😅 cc: @marcellamaki)

@marcellamaki and @jtamiace will tell better about the expected behaviour of this. Right now the current implementation is that the user needs to click on the search button to open the search results.
Im not being able to replicate this, I have tried it on my browser but it seems the line is breaking correctly. Could you provide more details how can I reproduce it?

pcenov · 2025-01-22T15:23:08Z

Thank @AlexVelezLl,
Here are my comments for the issues we are discussing:

I have reported a follow-up issue here Manage lesson resources > Search - No indication that there are no imported resources on the device #13021
I confirm that this is fixed now.
For points 3, 4 @marcellamaki pls let us know how to proceed. A note that in the Library page we are showing the results to the user immediately after applying the filter so that's my expectation for this search too. :)
@AlexVelezLl in order to replicate the issue you need to check it in an actual mobile phone as the problem is caused by the device's taskbar. I have double checked that this happens consistently. :)

github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Jan 15, 2025

AlexVelezLl requested review from marcellamaki, nucleogenesis and rtibbles January 15, 2025 22:46

AlexVelezLl added 8 commits January 16, 2025 08:39

Fix SearchFilterPanel showing a blank div

df5a7ec

Integrate Search Filters and Search Results to LessonResourceSelection

20fa5e8

Support browsing search result topics

c5466d5

Immersive side panel when browsing search result topics

6d11a58

Improve styles in search filters subpage

ac2f6a9

Fixes

11595e2

Refactor and add comments on some methods

f0f39d6

Move strings to searchAndFilterStrings and fix styles

f982e74

AlexVelezLl force-pushed the integrate-search-resource-selection branch from c731c48 to f982e74 Compare January 16, 2025 15:04

Optimize search requests

f7d1030

Allow single select mode in acordions and open by default item

2accb25

nucleogenesis reviewed Jan 20, 2025

View reviewed changes

AlexVelezLl added 2 commits January 20, 2025 16:27

Add title and go back method when selecting a category

610a6e7

Align Search Filter Panel styles with figma

c13df0e

pcenov mentioned this pull request Jan 22, 2025

Manage lesson resources > Search - No indication that there are no imported resources on the device #13021

Open

AlexVelezLl added 2 commits January 24, 2025 09:08

Fix styles

b44c2b1

Redirect to search results on filter change

26db2b8

AlexVelezLl force-pushed the integrate-search-resource-selection branch from 22dc161 to 26db2b8 Compare January 24, 2025 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate search resource selection #13008

Integrate search resource selection #13008

AlexVelezLl commented Jan 15, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

marcellamaki commented Jan 16, 2025

pcenov commented Jan 20, 2025

nucleogenesis left a comment

nucleogenesis Jan 17, 2025

nucleogenesis Jan 18, 2025

AlexVelezLl Jan 21, 2025

nucleogenesis Jan 23, 2025

nucleogenesis Jan 18, 2025

AlexVelezLl Jan 21, 2025 •

edited

Loading

nucleogenesis Jan 23, 2025

nucleogenesis Jan 20, 2025

AlexVelezLl Jan 21, 2025

AlexVelezLl commented Jan 21, 2025

pcenov commented Jan 22, 2025

Integrate search resource selection #13008

Are you sure you want to change the base?

Integrate search resource selection #13008

Conversation

AlexVelezLl commented Jan 15, 2025 • edited Loading

Summary

References

Reviewer guidance

github-actions bot commented Jan 16, 2025 • edited Loading

marcellamaki commented Jan 16, 2025

pcenov commented Jan 20, 2025

nucleogenesis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexVelezLl Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexVelezLl commented Jan 21, 2025

pcenov commented Jan 22, 2025

AlexVelezLl commented Jan 15, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

AlexVelezLl Jan 21, 2025 •

edited

Loading