-
Notifications
You must be signed in to change notification settings - Fork 727
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
Add searchFiltersPanel to Lessons #12871
Add searchFiltersPanel to Lessons #12871
Conversation
@@ -160,6 +192,10 @@ | |||
context: | |||
"In the 'Manage lesson resources' coaches can add new/remove resource material to a lesson.", | |||
}, | |||
searchFiltersTitle: { | |||
message: 'Search', | |||
context: 'TO DO', |
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 the context for this somewhere specifically written? could someone guide me on what should go here?
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 can use the searchLabel
in commonCoreStrings
for this, rather than add a new string here.
the context is a message that appears in the GUI of the app we use for translators who are translating Kolibri, and it helps them understand the, well, context, of the string! Sometimes Radina adds these in, but you can also take a "first attempt" for any strings that you do add (and she can edit them later). Details about where the user might encounter the text, or how they're interacting with it, can be a good place to start.
here's an example:
searchForUser: {
message: 'Search for a user...',
context:
'Text which appears in the search field above the table with users from whom to choose from (e.g. when enrolling learners to a class, selecting users to sync, etc.)',
},
const showSearch = ref(true); | ||
const topic = ref(null); | ||
|
||
const { searchTerms } = useBaseSearch(topic); |
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.
Could refine this further! Any guides on what to change and what not to here?
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.
there will be more to add, which I think you can come back to do the integration for. But for now, this is sufficient :)
Build Artifacts
|
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 @ozer550 thank you for this! It is just about ready to go, no major changes needed, but I added a small request about strings (and answered your question there about context) as well as a "you could do this if you want to add it on!" about the navigation. That bit will require some follow up with designers.
let me know if you have questions about any of my comments/feedback
@@ -8,69 +8,88 @@ | |||
@shouldFocusFirstEl="() => null" | |||
> | |||
<template #header> | |||
<h1 class="side-panel-title">{{ $tr('manageLessonResourcesTitle') }}</h1> | |||
<h1 class="side-panel-title"> | |||
{{ !showSearch ? $tr('searchFiltersTitle') : $tr('manageLessonResourcesTitle') }} |
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 is okay for now, but I think we may end up making some changes here when we update the side panel component a little bit, to make the title here more dynamic. (this is not blocking and just an FYI in case you see changes here later, or I ask you to make them in an upcoming issue 😄)
const showSearch = ref(true); | ||
const topic = ref(null); | ||
|
||
const { searchTerms } = useBaseSearch(topic); |
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.
there will be more to add, which I think you can come back to do the integration for. But for now, this is sufficient :)
@@ -160,6 +192,10 @@ | |||
context: | |||
"In the 'Manage lesson resources' coaches can add new/remove resource material to a lesson.", | |||
}, | |||
searchFiltersTitle: { | |||
message: 'Search', | |||
context: 'TO DO', |
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 can use the searchLabel
in commonCoreStrings
for this, rather than add a new string here.
the context is a message that appears in the GUI of the app we use for translators who are translating Kolibri, and it helps them understand the, well, context, of the string! Sometimes Radina adds these in, but you can also take a "first attempt" for any strings that you do add (and she can edit them later). Details about where the user might encounter the text, or how they're interacting with it, can be a good place to start.
here's an example:
searchForUser: {
message: 'Search for a user...',
context:
'Text which appears in the search field above the table with users from whom to choose from (e.g. when enrolling learners to a class, selecting users to sync, etc.)',
},
if (this.showSearch == true) { | ||
this.$router.go(-1); | ||
} else { | ||
this.showSearch = 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.
So, this was not explicitly listed as acceptance criteria, but in the figma, this panel actually has a back arrow and a close button. Can you check in with @jtamiace about the expected behavior, and potentially update your PR to include this? I think what should happen is that the back arrow should return the user to their previous location, and the close arrow should close the side panel entirely. (cc @nucleogenesis)
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.
Thanks @ozer550 ! One note - the side panel close button seems to be sometimes functioning like "back". We are going to be refactoring the side panel anyways, and this will integrate with some work in progress Alex has, so I think it's okay for now. Thank you!
Summary
References
closes #12788
Reviewer guidance
/coach/#/:classId/lessonstemp/:lessonId/select-resources.