-
Notifications
You must be signed in to change notification settings - Fork 713
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
Quiz creation resource selection: Topic selection & "Select all" #11864
Quiz creation resource selection: Topic selection & "Select all" #11864
Conversation
2204222
to
e40d90e
Compare
Build Artifacts
|
@@ -296,6 +400,8 @@ | |||
resetWorkingResourcePool, | |||
contentPresentInWorkingResourcePool, | |||
//contentList, | |||
cannotSelectTopicCard$, | |||
cannotSelectSomeTopicWarning$, |
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 also want to note that this string is showing at up at the first level when I use back navigation to navigate back . In addition, I tested with QA channel and tigil-fajod
but one will not show up at the end of the routes when using the back button to exit the navigation except the active working resource pool .
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 that once #11862 is merged with this that this will be resolved - I'll make note of this to check it specifically in testing after merging.
@@ -366,6 +458,12 @@ | |||
}, | |||
}, | |||
methods: { | |||
showTopicSizeWarningCard(content) { |
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 makes me think that we need to properly update this method to handle to topic selection- for now I think it's getting lost in the way we are handling back route navigation.
<div | ||
v-if="showTopicSizeWarning()" | ||
class="shadow" | ||
:style=" { padding: '1em', backgroundColor: $themePalette.grey.v_100 }" |
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.
Just curious how this will look in the new LE's theme.
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.
Yeah that's a good point - @jtamiace mentioned to me that the colors palette will be reduced a bit so I think uses of the palette like this might need to be updated when that happens.
- Implements topic selection logic (can select topic when all children are fetched and are exercises) - [De]select all accounts for possibility of topics, using new `hasCheckbox` logic - Only shows "Select all" when every child `hasCheckbox` based on the new logic - Generally begins moving logic into relevant blocks within setup()
e40d90e
to
ecb5920
Compare
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.
Looks good! The navigation business will be handled in #11862 . Thanks @nucleogenesis
Summary
hasCheckbox
logichasCheckbox
based on the new logicReferences
Closes #11790
Reviewer guidance
Messaging
Select all logic