-
Notifications
You must be signed in to change notification settings - Fork 686
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 SearchFiltersPanel for Coach #12759
Update SearchFiltersPanel for Coach #12759
Conversation
Build Artifacts
|
@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:
|
fdc0c79
to
e0d300c
Compare
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:
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.
kolibri/kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionSidePanel.vue Lines 57 to 69 in 4bda163
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 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?"
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:
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: 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 😂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I have just left some questions that were not entirely clear to me 😅
@@ -67,7 +67,7 @@ | |||
</div> | |||
</div> | |||
|
|||
<div v-if="!isTopicIdSet && bookmarks.length && !showBookmarks"> | |||
<div v-if="!isTopicIdSet && bookmarks.length && !showBookmarks && !showSearch"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Non blocking] The render conditions of this component are getting each time more complex, it would be good if in the future we can find a way of making this more easy to follow. Perhaps breaking down this component into more specific use cases? like BookmarksResourceSelection
, SearchResourceSelection
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes for sure! All of the changes here to ResourceSelection are basically for demonstration purposes - although @marcellamaki and I are drafting an issue which will hopefully refactor this component quite a bit
|
||
<div> | ||
<AccordionContainer | ||
v-if="Object.keys(availableLibraryCategories).length" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a posibility that all these conditions arent met and we have an empty div for AccordionSelectGroup
? I suspect if we dont have any of these we wouldnt even get to this point, but just curious if we have considered this.
}, | ||
methods: { | ||
isChecked(inputKey, value) { | ||
return this.isSelected(inputKey, value) || this.isEnabledButNotSelected(inputKey, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Marcella's comment about disabled selected values is because we are considering this isEnabledButNotSelected
value as correct value for isChecked
, and I wonder, why would we check something if its not selected, even if its enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just makes it so that you see the changes to what is selected even if it's not directly clicked by the user (ie, you select Spanish, then the other available filters are disabled/checked-automatically if there are no Spanish resources for them).
My latest updates make use of this (fixing the issue where everything is checked & disabled because I misread/understood the enabled computed properties).
However, I can probably just remove the isEnabledButNotSelected
altogether if we handle selections like we do in Learn because part of the issue is to do with a reactivity issue somewhere if I recall correctly.
v-for="a11y in accessibilityOptionsList" | ||
:key="'a11y-' + a11y.value" | ||
:checked="isChecked('accessibility_labels', a11y)" | ||
:disabled="a11y.disabled || isEnabledButNotSelected('accessibility_labels', a11y)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont understand that much these conditions. We disable it if its enabled but not selected? I found it a little bit confusing 😅
} else { | ||
this.$emit('input', { | ||
...this.value, | ||
[field]: { ...prevFieldValue, [value.value]: true }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this, the channels are working as a checkbox rather than as a radio button, and we can select both of them but just one appears as selected
I wonder if we should change the radio buttons to be checkboxes or change this handleChange
method to support the radio button behaviour to have just one selected value and remove the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Alex, I've resolved this issue
position: absolute; | ||
top: 50%; | ||
left: 0.5em; | ||
transform: translateY(-50%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed to center the icon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep!
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):
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. |
e0d300c
to
4e7e96f
Compare
4e7e96f
to
34bba61
Compare
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: ActivityButtons.flow.mp4For 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... checkboxes.and.filters.mp4Also if there are no options that can be selected from a filter, shouldn't it be completely disabled: not.selectable.mp4Lastly 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
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 )
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. |
5997627
to
6b99dfd
Compare
Hi @nucleogenesis, I think it looks much better now! Here are the latest issues I was able to spot:
unrecognized.mp4This 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.
categories.mp4
no.back.arrow.mp4
language.mp4 |
b4ece1f
to
c673a02
Compare
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nucleogenesis, I confirm that the issue with the filters showing up at the bottom of the page is fixed now.
No new issues observed while manually testing, so I'm going to approve this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of small tweaks that I think will make the search integration much more complete within the scope of this PR.
ResourceSelectionBreadcrumbs, | ||
}, | ||
mixins: [commonCoreStrings], | ||
setup(props, context) { | ||
const { searchTerms, search } = useBaseSearch({}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are already defining the topic
below, we should pass that in as the descendant
option to useBaseSearch
to properly hook this up to the descendant restricted searching when applicable.
const { searchTerms, search } = useBaseSearch({}); | ||
// Search if we already have search terms when we load up | ||
if (flatMap(searchTerms.value, term => Object.keys(term)).length) { | ||
search(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just call search
unconditionally, like we do in the library page: https://github.com/learningequality/kolibri/blob/develop/kolibri/plugins/learn/assets/src/views/LibraryPage/index.vue#L515
(although I am not quite sure why we are calling it in the created hook and not the setup function).
…election; search unconditionally
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good, let's unblock this!
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