Section settings side panel #11314
Section settings side panel #11314
Conversation
github-actions
bot
added
APP: Coach
Build Artifacts
|
|
Hi @AllanOXDi, could you please address the conflicts reported here? |
github-actions
bot
added
DEV: backend
There was a problem hiding this comment.
Hi @AllanOXDi - great work so far! It's really exciting seeing this work coming together. I've added some feedback in terms of some general code adjustments. The two main blockers for me right now are in regards to strings and swapping out hex color code for theme tokens.
I realize that a lot of this work is about getting the foundations of a component in place, but not necessarily all of the functionality, so I will defer to @nucleogenesis in regards to some of my comments about "not using placeholders" such as in strings and router paths. If you two have a plan for this approach, that's totally okay. But, one thing I am reflecting on a lot recently is that sometimes stubbing things out can mean that things get missed or problems don't surface until later on in the development process. At the same time, working out every detail before merging PRs can slow things down. I'd encourage you and Jacob to at least chat through the pros and cons together as you decide what is blocking and what can be done in follow up. (But really I would so greatly prefer that we do not merge placeholder strings if at all avoidable 😄)
Thanks again for all of the hard work here! It's great to see you contributing so much to this new feature. And please do not hesitate to reach out if there are any comments that I can clarify or help with 🙂
|
|
||
| <KRouterLink | ||
| appearance="raised-button" | ||
| :to="{ path: 'new/123/replace-questions' }" |
There was a problem hiding this comment.
I'm not sure what these paths should be based on the architectural decisions made with @nucleogenesis, but I think if it's possible for these to be the real path (or alternatively, maybe they should be just a button and not a router link at all), it would be worth trying to work that out now and get as much of the behavior in place as possible, even if the actual "saving" and "replacing" to the database doesn't happen until later
kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionEditor.vue
Show resolved
Hide resolved
| @@ -4,6 +4,7 @@ | |||
| v-if="$route.params.section_id" | |||
| ref="resourcePanel" | |||
| alignment="right" | |||
| sidePanelWidth="700px" | |||
There was a problem hiding this comment.
While this is probably the width that is listed on the spec, it will be too big on smaller screens. This should be updated so that the width is 700px on larger screens, and smaller on small screens. In other places, we have a full-screen panel on mobile view, for example, where the width is 100% of the screen width. Worth confirming with @tomiwaoLE that this is the approach that would be correct for the designs, and also checking with @nucleogenesis about to implement (and if there is any logic we can repurpose from other places where we solve the same problem)
There was a problem hiding this comment.
Hi @tomiwaoLE ,I am dropping by to find out the intended side panel behaviour on a mobile device, could it be that it should occupy half the screen size or can take full screen ?
There was a problem hiding this comment.
@AllanOXDi IIRC the side panel goes full width on mobile - you can double check by opening it for a piece of content in the library
| numQuestions: 1, | ||
| }; | ||
| }, | ||
| $trs: { |
There was a problem hiding this comment.
I have two high-level comments about strings here.
First, rather than put strings in the component and use strings in the component, we can now use the strings in enhancedQuizManagementStrings (see kolibri-common/strings/enhancedQuizManagementStrings.js). Based on the team feedback, we are going to try this out for this project. I know we were a bit delayed in getting these in, but I think having just one place for strings will hopefully make things easier. I think it is worth refactoring the strings here now, rather than merging these in and having to do the cleanup later. @nucleogenesis can probably do a cohack session with you (and @thanksameeelian if she is curious - I know she has some frontend work coming up) about how we will be accessing and using these strings within the components. Hopefully it will be easier. And, any edits that you need to make to the strings can be made directly to that file.
Secondly, and relatedly, I understand that you are just scaffolding out the UI here, but I think we should try to refrain from adding placeholder strings (such as "2 resources selected from 2 channels") unless they are absolutely necessary. I realize that the scope of this PR might not include all of the functionality that would enable some of the interactions on this section of the page, but I would prefer that use the real counts (i.e. if we don't have any selected resources, just have it be the data which would be 0) rather than pass in placeholder strings or placeholder values. I'm not sure I'm explaining this very well, so please do ask if you have questions about what I mean.
| .section-order-list { | ||
| height: 2.5em; | ||
| margin-top: 0.5em; | ||
| border: 1px solid #dedede; |
There was a problem hiding this comment.
For all of the colors throughout the css here, every time you are using a hex code, please swap this out with one of our theme tokens. To do this, it will mean adding it in the template, or calculating in computed as needed, rather than setting it here in the css. But, it means that our theming will not break :) There are several places throughout this PR where this is used, and this update should be applied to all of them.
kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionEditor.vue
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks @AllanOXDi! Code generally looks good. Also, I generally agree with Marcella's feedback on the areas she has pointed out that need improvement so it might be good to have these in before the merge.
github-actions
bot
added
SIZE: very large
DEV: backend
| :primary="true" | ||
| :text="applySettings$()" | ||
| class="apply-settings-style" | ||
| @click="quizForge.saveQuiz()" |
There was a problem hiding this comment.
This shouldn't call on the saveQuiz but rather should be calling updateSection. saveQuiz will be connected to the "Save" button on the root page and commits changes to the server but updateSection will only update the quizForge state
| sectionOrderList: [ | ||
| { | ||
| name: 'Section order', | ||
| current: this.currentSection$(), | ||
| isActive: true, | ||
| }, | ||
| { | ||
| name: 'Section 2 / unique title', | ||
| current: null, | ||
| isActive: false, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
I think this ought to probably be a computed property on this that reads from quizForge.allSections. I'll note above re: how/where the section order should be updated.
| }, | ||
| }, | ||
| methods: { | ||
| handleSectionSort() {}, |
There was a problem hiding this comment.
This will need to call quizForge.updateQuiz so that the question_sources property reflects the updated sort order. Combining this with populating the drag container w/ the quizForge.allSections will get this working I think.
| sectionSettings$, | ||
| sectionTitle$, | ||
| numberOfQuestionsLabel$, | ||
| optionalDescriptionLabel$, | ||
| quizResourceSelection$, | ||
| numberOfSelectedResources$, | ||
| currentSection$, | ||
| deleteSection$, | ||
| applySettings$, | ||
| changeResources$, | ||
| sectionOrder$, | ||
| questionOrder$, | ||
| randomizedLabel$, | ||
| randomizedOptionDescription$, | ||
| fixedLabel$, | ||
| fixedOptionDescription$, |
There was a problem hiding this comment.
Non-blocking thought here re: this new string convention (cc @marcellamaki @rtibbles )
I'm feeling of two minds on this convention where we destructure all of the message partials out and then return them in setup. With a lot of strings like this, it just adds 30 lines of cruft right at the top of the component definition.
So I'm wondering then if maybe we should, by convention, just return the strings object like:
setup() {
/* ... */
return {
eqmStrings: enhancedQuizManagementStrings,
/* ... */
}
Then we'd use it like this.eqmStrings.sectionLabel$() or {{ eqmStrings.randomizedLabel$() }}.
Just feels like the destructuring of the strings, as a pattern, would be better suited for when you're pulling one string out of the common core, but when you're pulling in a full feature's strings then you're liable to be using quite a lot of them.
In any case -- just want to get some thoughts.
There was a problem hiding this comment.
Sounds like a battle between:
Explicit is better than implicit.
and
Sparse is better than dense.
or
Beautiful is better than ugly.
The laundry list doesn't particularly bother me, simply because of the explicitness involved, it tells us exactly which strings are going to be used in the component, and the component doesn't have to have any knowledge beyond the setup function, as to where those strings have come from.
There was a problem hiding this comment.
I was also wondering if a component that has a lot of strings will mean initializing the string from the setup and calling them the template or javascript will just increase the number of lines of code in the component.
I think your approach @nucleogenesis would be good to limit that.
| sectionOrderList() { | ||
| return [ | ||
| { | ||
| name: 'Section order', | ||
| current: this.currentSection$(), | ||
| isActive: true, | ||
| }, | ||
| { | ||
| name: 'Section 2 / unique title', | ||
| current: null, | ||
| isActive: false, | ||
| }, | ||
| ]; | ||
| }, | ||
| }, | ||
| methods: { | ||
| handleSectionSort() { | ||
| this.quizForge.updateQuiz(); |
There was a problem hiding this comment.
You'll need to pass something to update here.
So, that @sort event that calls this handleSectionSort will pass a value (I think just the new version of the list values that have been sorted) -- you'll need to make sure that the list has objects that validate with the type defined as a QuizSection. So here you should basically expect to receive a parameter in this function.
The shape of the data given to this parameter, I believe, will be the same shape you define in sectionOrderList (which should be returning quizForge.allSections) -- so this means that if you're using that allSections list, which is a list of objects in this shape, then you can just take the value of that given parameter and pass that to the updateQuiz call.
Note how on the Quiz typedef you'll see there is a question_sources property which is where we store the quiz's list of QuizSection objects.
This means that the updateQuiz needs an object with keys that map to the Quiz type. In this particular case, you should be able to take in the param passed to handleSectionSort and then pass { question_sources: theParamThatWasPassedIn } to update the order of the sections.
| :style="{ color: $themePalette.grey.v_700 }" | ||
| > | ||
| <p class="current-section-text space-content"> | ||
| {{ section.current }} |
There was a problem hiding this comment.
Related to my comment about handling the @sort event below -- since the sectionOrderList will be returning an array of QuizSection objects, we'll have the section_id on hand and can just check to see if quizForge.activeSection is equal to the section iterated over here.
| section_id: this.quizForge.activeSection.value.section_id, | ||
| section_title: this.sectionTitle, |
There was a problem hiding this comment.
The final stretch here will be wiring up the radio buttons for the randomization, question count, and description and this should be all set here
There was a problem hiding this comment.
A related issue is that the section's current settings aren't being applied to the form when it is initialized. For example, the first default created section has the title "Section 1" which should populate the "Title" textfield.
| <Draggable | ||
| v-for="(section,index) in sectionOrderList" | ||
| :key="index" | ||
| > |
There was a problem hiding this comment.
I think we should put a background: $themeTokens.surface CSS value on this because otherwise, the background is transparent and you can see through it as you drag it making it kind of look/feel insubstantial in a weird way. (I just realized this while using the draggable stuff w/ the accordion)
nucleogenesis
dismissed
marcellamaki’s stale review
Will address the hard-coded side panel width elsewhere. Other feedback has been addressed
Summary
Built Quiz Section Settings Side Panel.
References
closes #11016
Reviewer guidance
See the figma design.
Testing checklist
PR process
Reviewer checklist
yarnandpip)