Closed nucleogenesis closed 1 week ago
Asset type | Download link |
---|---|
PEX file | kolibri-0.18.0.dev0_git.20241114210926.pex |
Windows Installer (EXE) | kolibri-0.18.0.dev0+git.20241114210926-windows-setup-unsigned.exe |
Debian Package | kolibri_0.18.0.dev0+git.20241114210926-0ubuntu1_all.deb |
Mac Installer (DMG) | kolibri-0.18.0.dev0+git.20241114210926.dmg |
Android Package (APK) | kolibri-0.18.0.dev0+git.20241114210926-0.1.4-debug.apk |
TAR file | kolibri-0.18.0.dev0+git.20241114210926.tar.gz |
WHL file | kolibri-0.18.0.dev0+git.20241114210926-py2.py3-none-any.whl |
Unfortunately, the search sidebar comes up empty for me on both Firefox and Chrome in Windows 10 😕 (there are channels with exercise resources on the device)
Firefox | Chrome |
---|---|
@radinamatic only on windows 10?
Apologies, this looked to me like something that would not be influenced by the OS type, so did not think to check yesterday...
Just confirmed that I can equally replicate in Firefox and Chrome on Ubuntu 20.04:
Firefox | Chrome |
---|---|
https://github.com/user-attachments/assets/767cef54-9ebe-4ace-9800-96d4303a0bd0 |
I've run into a couple issues getting a few things exactly to spec which I will detail here.
There are a few places that we should probably consider refactoring sooner than later I think -- namely:
1) Uses of SearchFilterPanel
in Learn - There is a bit of a balancing act between the two depending on whether windowIsLarge
or not. SearchFilterPanel
will render a SidePanelModal
when !windowIsLarge
which makes SearchFiltersPanel
a little wonky to "drop-in" somewhere. Then in the TopicsPage
, the side panel has to live in a particular fixed position when windowIsLarge
.
I think that this can be done w/ a little bit of reworking to how SearchFiltersPanel renders itself and updating TopicPage and LibraryPage to handle the responsibility of things like how/where it is shown depending on the window size. I started down the path of trying this while working on this PR and it's going to be a little bit fiddly to get going.
2) SectionSidePanel - This was created as a wrapper for "when the side panel is open" during Quiz creation, but it ultimately required some overly specific handling of "when can the user go back or not":
The issue here is that because the Quiz side panel business is wrapped this way and the SearchFiltersPanel wants to be it's own little SidePanelModal
when !windowIsLarge
we get nested SidePanelModals
in mobile views and don't have a direct way to say "show the back arrow when in the 'choose a sub-category' view".
Basically, I got to where I'd have to update that watcher in SectionSidePanel and figured it might be worth removing that overly complicated code for answering the question "back arrow or close icon?"
3) The SidePanelModal should probably have a title
prop added where we can just put the text we want to see in the heading-area of the side panel - currently it uses a #header
slot (which I think could do well to be renamed to something like #sidePanelHeader
) -- but it's such a common part of the side panel designs now that it'd probably be best to standardize it.
So, that said, I have pushed updates that should be working @radinamatic -- I watched the Chrome video you linked and I'm wondering if this is to do with the mobile stuff I mentioned as the screen started small, then was widened. I ran into something similar to this today.
I've tested in mobile on Learn and Coach today and the main things that won't be correct are:
With regards to:
reworking to how SearchFiltersPanel renders itself and updating TopicPage and LibraryPage to handle the responsibility of things like how/where it is shown depending on the window size
I would say this is in scope of this issue. I guess in the issue itself I cautioned against refactoring, but that was really in terms of "how search works". Changes to support the conditional rendering make sense. If talking through some of the pros and cons to approach would be helpful, I'm happy to do that.
And, a couple of other things I noticed just looking in the browser:
This is only in coach, but it looks like everything is selected and inactive. I know that the actual integration of useSearch
is coming later, but this seems unintentional, even just for initial QA. Not sure if something just got a bit wonky here?
When making a selection, for example, when I choose 1 category, the other filters auto update and indicate that they are selected. On the one hand, this makes sense, because I think it means that only one option is possible for the other metadata fields, and therefore, the search is "including" these additional criteria. However, it feels a little odd to me, I guess because the search in the URL is strictly about the categories, not the other fields (see in screenshot). I'm not saying this is necessarily a blocker, maybe it is the expected behavior, or it would be technically complex to do it differently. But, I think we need to ask @jtamiace about whether this is what we want or not.
Design [nit-picks, sorry]: the "activity" buttons should be 4 across not 2; the dropshadow on the accordions is such a small thing but it is driving me bananas for a unknown reason 😂
Hi @nucleogenesis in addition to what's been already mentioned by you and Marcella, I was able to identify the just the following potential issues:
Content in the search sidebar is now visible, yey! 👍🏽
To add on comments by @pcenov (and I completely agree, the Activities section is completely superfluous here, since only the Practice type of resources can be used in quizzes):
Channels should be pre-filtered to only display those that contain exercises
I find it unclear and confusing that when a second level category is selected as filter, that selection is not clearly displayed: if I select to filter Mathematics
and I close the modal, only the School
is marked/highlighted. What is user supposed to do in order to review/remind themselves what they selected, open the modal again? 🤔
Go/Proceed/Search
button or such, after the user has made their various selections, to actually perform the search... 😕
If they type in a keyword they can use the ➡️ button, but if they want just to filter by other options without keywords, how do they actually initiate/perform the search? It's not clear to me from Figma...Keyboard navigation at this point does seem to be working correctly, but with many still moving parts and WIP, I'm reserving the judgment and will re-test as dev work progresses.
cc @jtamiace for questions 2 & 3.
Update on my points 2 & 3 above, as my concerns and confusion seems to be caused by the search not being performed at this stage. Once the current filtering workflow as in Learn > Library is fully implemented here, concerns will probably be resolved.
@radinamatic @pcenov
Some updates to review and/or things bear in mind:
(cc @marcellamaki)
Hi @nucleogenesis,
Seems that the changes made to the ActivityButtons flow are a step in the right directions but that's causing issues in the Library page and the Topic page:
https://github.com/user-attachments/assets/18e2350e-60b5-4dcf-8818-06337c7edcb5
For the checkboxes and radio buttons - can't we have the same drop-downs as the ones at the Library page? The current implementation where I select a checkbox and all the other checkboxes become disabled + other filters get disabled in the background is not very intuitive and seems too complicated? Also when the filters are collapsed it's easy to get lost and forget what were the selected values...
https://github.com/user-attachments/assets/9e4d3055-e3f4-422b-846d-8da14d9dc66a
Also if there are no options that can be selected from a filter, shouldn't it be completely disabled:
https://github.com/user-attachments/assets/9ee40df6-c430-415f-8a9d-86bf154c96e9
Lastly for the Categories filter I noticed that we are currently missing the Uncategorized option.
@pcenov I've updated to fix the design issues and the missing Uncategorized category
For the checkboxes and radio buttons - can't we have the same drop-downs as the ones at the Library page? The current implementation where I select a checkbox and all the other checkboxes become disabled + other filters get disabled in the background is not very intuitive and seems too complicated? Also when the filters are collapsed it's easy to get lost and forget what were the selected values...
I totally get that - do you think that this would be better if you were being taken to the search results after making a selection? This will be the UX - so the user won't see the filters selected-ness or disabled-ness unless they re-open the search panel after their results are shown. (cc @jtamiace @marcellamaki )
Also if there are no options that can be selected from a filter, shouldn't it be completely disabled:
I think so but this would take some updates to the Accordion - we could probably disable the button that toggles the Accordion but I'm not super sure if that'd work.
Hi @nucleogenesis, I think it looks much better now! Here are the latest issues I was able to spot:
https://github.com/user-attachments/assets/a96f429c-9a1f-46e6-90cb-eaf88ac69307
This is also valid when I select that option at the Library/Topics page but there we have the 'Clear all' and delete filter options. So if possible clicking on "Uncategorized" should first enable the filter and then clicking again on it should remove the filter.
https://github.com/user-attachments/assets/98abaa01-ccdb-427b-835f-126b75949714
https://github.com/user-attachments/assets/226e4629-6f3c-4ce2-9924-3e03991867d8
https://github.com/user-attachments/assets/7ad16f0f-5a52-48a7-a617-cfdf0f400b25
@pcenov I chatted w/ Marcella briefly and she though to try an "All channels" sort of 'default' option for the channels radio buttons. Could you try that out and let us know what you think? It should only be present on the accordion version of things.
I've also:
Uncategorized
button toggle whether or not it is selectedYour other notes re: the back arrow are noted and will be done in a follow-up issue - I did some work toward this in #12819
Hi @nucleogenesis,
I confirm that now the Uncategorized
option can be deselected at the Quiz page, so all good on that front.
The removal of the Channel
drop-down looks like a very neat idea to me. The only problem I am seeing is that if I select a channel and then attempt to filter the results I am still seeing all options, while in that case the filters should be only for the selected channel (same as the current implementation at the topics page):
https://github.com/user-attachments/assets/ba0934d0-7824-4c22-b4b9-9028b1281fff
Categories
modal is not loading nicely as before at the Library/Topics page (all supported browsers). While it's loading I can see the outlines of the cards for a brief moment:https://github.com/user-attachments/assets/c61a3c7d-6175-4bf8-a2fd-0edd3601f4d6
@nucleogenesis just a note that the design issue with the filters at the Library page is still not fixed:
https://github.com/user-attachments/assets/6048ed5d-9f61-4c8c-a978-a9ddb16a2cb9
This issue mentioned above is also still extant in the latest build:
"if I select a channel and then attempt to filter the results I am still seeing all options, while in that case the filters should be only for the selected channel (same as the current implementation at the topics page)":
https://github.com/user-attachments/assets/ba0934d0-7824-4c22-b4b9-9028b1281fff
@marcellamaki @pcenov I've fixed the issue w/ the filters showing up at the bottom of the page.
re: the channels filters being applied as the user navigates coach, I think that'll be done in #12819 when things get wired up in full
Thanks @rtibbles I've updated LibraryPage to call search in setup, passed the topic ref in for the descendant param & call search unconditionally in ResourceSelection.
Summary
SearchFiltersPanel
to accommodate latest design changes which will be used in the quiz resource selectionFollow-up tasks (which I can do here and/or make an issue for):
key
used in Kolibri were the label for the icon that'd be helpful)ActivityButtonsGroup
component could use an update that lets use modify how they flow on the screen. We could fit 3, maybe 4 per row in the side panel but they display only in two columns. Perhaps some flex styles could do us well here to serve all use casesReferences
Closes #12521
Reviewer guidance
Feature QA
NOTE: This is largely design focused, so any functionality issues will be addressed when it is fully integrated into Coach
Regression testing
a11y guidance, keyboard nav in particular
I have added
tabindex=-1
to the KIconButton for the chevron to the right on the accordion heading area, so you will go from one accordion to the nextaria-expanded
should be set as expected