-
Notifications
You must be signed in to change notification settings - Fork 716
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 foundations & data connection #11277
Quiz foundations & data connection #11277
Conversation
Build Artifacts
|
4936295
to
0651d0c
Compare
@@ -124,6 +124,13 @@ export function useQuizCreation() { | |||
if (updatedSections.length === get(allSections).length) { | |||
throw new Error(`Section with id ${section_id} not found; cannot be removed.`); | |||
} | |||
if (updatedSections.length === 0) { | |||
console.log('Deleting last section...'); |
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 console.log can be removed.
@@ -240,6 +247,10 @@ export function useQuizCreation() { | |||
const activeSection = computed(() => | |||
get(allSections).find(s => s.section_id === get(_activeSectionId)) | |||
); | |||
/** @type {Arrayc<ComputedRef<QuizSection>>} The inactive sections */ | |||
const inactiveSections = computed(() => |
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.
Nice
watch: { | ||
$route: function(_, o) { | ||
this.prevRoute = o; | ||
console.log('prevroute', this.prevRoute); |
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.
The same here.
@@ -491,19 +296,25 @@ | |||
<style lang="scss" scoped> | |||
|
|||
.style-icon { | |||
width: 2.5em; | |||
height: 2.5em; |
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.
What could be the reason for not maintaining em units ? as we discussed.
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.
Old habits die hard 😅 - will update to use em
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 all looking pretty good to me! i followed the reviewer guidance and navigated through the available components a few times, successfully added and deleted sections from the beginning, middle, and end, seeing the titles update & the ids be correct, opening the side panel, etc., and i found it was all (or nearly all) working as i expected!
i'm interested in the outcome of @AllanOXDi's comments, and the ones i'll leave here are all UX-related. i'd like to call out these concerns here to make sure they're visible, but many may be better handled in a separate, follow-up PR (or just dismissed haha)
-
i was thinking that if you add a new section, perhaps that section should be automatically focused? but that is just an idea/preference that can be easily rebutted by the design team or by you if you don't feel like that's the behavior you'd like :)
-
i think there is a little bit of focus-ring-funkiness going on, which is probably best displayed in the attached screen recording:
-
when you tab through the page, the
X
closing button for the whole page is only visibly focused on the first pass through. if you tab back around to it again, it is focused (at the end of the video i trigger the invisibly-focusedX
with my keyboard and successfully close the page), but the only visual indication is the URL preview at the bottom of the window. -
the focus rings around the section names are currently displaying oddly, and when focus moves to the Edit/Delete menu-triggering button it isn't very clear - it appears that parts/sides of the outline are missing, particularly the side that abuts the next section name. the last section in the list has the most complete outline, but it is still a bit unexpected-looking
-
focus-indicators.mov
- just a comment about something that feels weird to me, which is the appearance of hovering over a currently-selected-section's name:
i just find this visually a little odd. it makes sense that just the name would be showing as hovered/greyed, as the button beside it has a separate hover state which also makes sense, but the disconnect between the underline (of the whole containing div) and what is highlighted in the hover state keeps catching my eye. not sure if there is any possible "better" way to do it though!
💯 for sure will update accordingly To the rest of your comments, which I think are all due to the same weirdness of there being a The wrapping is due to how the I'll also look at the focus ring w/ regard to the page closing X button. Thank you so much for all of your notes! |
@thanksameeelian @AllanOXDi I've addressed most of the feedback here -- the main thing I haven't addressed here is the fix for the tabs. I've begun applying that fix in my ongoing work to close #11274 |
further cleaning futherfurther cleanup
7472252
to
528fb28
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.
The changes have been addressed !
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.
LGTM!
so much is being introduced here & i'm so excited to see it taking shape. on top of everything else, thanks for your attention to detail & for keeping an eye on that tab issue going forward!
Summary
useQuizCreation
module to the UI in many places (noted below)To be done in follow-up issues
References
Figma
Reviewer guidance
quizForge
- good or not good name?No critical testing needed here yet - this should be merged relatively quickly to unblock follow-up work.