Fix Quiz expanding between questions #2184

itsyme · 2023-02-23T06:02:09Z

What is the purpose of this pull request?

Overview of changes:
Resolves #2126. Forces intro/outro cards and questions in Quiz to wait for the previous card to leave before allowing the next card to enter.

Anything you'd like to highlight/discuss:
As a side effect of this PR, gotoNextQuestion in Quiz.vue had to be refactored into two functions,gotoNextQuestion and showNextQuestion, in order for the hiding of the current question and the showing of the next question to be called in two separate places. The transition duration was also reduced to keep the same effect as before.

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Fix expanding of Quiz during transition

Cards in Quiz expands during transition from one to the next due to
both cards being rendered in the Quiz.

This could be undesirable as it is an unexpected behaviour for users
and could lead to confusion when doing the Quiz.

Let's make the next card in the Quiz wait for the previous card to
leave before entering.

This will prevent the expansion of the Quiz component as it is
guaranteed that the Quiz will only have one card rendered at any
one time.

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

yucheng11122017

Hi @itsyme, thanks for the work on this!
I think the reason why the things in Quiz.spec.js.snap is being deleted is because it hasn't rendered yet after the triggers are triggered. I added await quizTimeOut() to lines 198 and 237 in Quiz.spec.js and it seems to work.
Also I was wondering how did you get 525 for the value of delay?

packages/vue-components/src/questions/Quiz.vue

packages/vue-components/src/__tests__/Quiz.spec.js

itsyme · 2023-02-23T07:02:03Z

Thanks @yucheng11122017! I got 525 by experimenting. I tried using 500 since the transitions were 0.5s long under styles -> .question in Question.vue, however it seemed to be too short so I increased it till it achieved the desired result!

itsyme · 2023-02-23T07:19:24Z

Hi @itsyme, thanks for the work on this! I think the reason why the things in Quiz.spec.js.snap is being deleted is because it hasn't rendered yet after the triggers are triggered. I added await quizTimeOut() to lines 198 and 237 in Quiz.spec.js and it seems to work. Also I was wondering how did you get 525 for the value of delay?

Thank you for the feedback! I added await quizTimeOut() to those two lines and it works perfectly now!

ang-zeyu

There are some flickering issues introduced by this change.

Presumably because the timeout is timed to be slightly after the transition actually ends.

Consider trying:

something better guaranteed https://developer.mozilla.org/en-US/docs/Web/API/Element/transitionend_event (what Vue internally uses also actually)
if that alone dosen't work.. (i have no time to browse the spec to check if the new card is introduced right before or after the transition ends...) consider if you really want to be attaching the new card only after the transition of the previous card has finished. (I am personally fine with this, though this changes the UX)

I believe there are just 2 main cases to consider:
1. New card's height > old card's height
2. Old card's height > new card's height
What can you do in these scenarios?

The implementation details should be clear after.

ang-zeyu · 2023-02-23T13:44:03Z

packages/vue-components/src/__tests__/Quiz.spec.js

+// Function to delay test until question is rendered
+export default function quizTimeout() {
+  // eslint-disable-next-line no-promise-executor-return
+  return new Promise(resolve => setTimeout(resolve, 525));


Timeouts are an easy fix to many issues at first glance but we are making assumptions on browser internals and user device capabilities that are never guaranteed.

(Not to say they should never be used.. there are certainly good use cases, but I strongly don't think this is one of them)

itsyme · 2023-02-24T04:44:31Z

Hi @ang-zeyu! Thank you for the feedback! I will try out your suggestions in the next commit!

itsyme · 2023-02-24T10:13:30Z

I managed to overcome this issue by using position: relative for the animation when entering, this allows the incoming question to render in the same place as the previous question. However, since the intro and outro cards are not questions and were not animated, I had to add an animation to them in order to remove the expansion from intro -> first question and last question -> outro. I also realised that the progress bar is being blocked again during the animation from intro -> first question and outro -> first question (when retrying) and am currently working to fix that. I am also considering changing the name of intro-outro-card under style to something more relevant.

ang-zeyu · 2023-02-25T06:25:10Z

Not sure if it has been updated but there is still some flicker currently:

itsyme · 2023-02-25T12:07:32Z

Hi @ang-zeyu! Hmm that is odd. Since I made the change I have not observed this behaviour. May I know which browser you are using so that I may further investigate this issue? Thank you!

ang-zeyu · 2023-02-25T16:23:22Z

Hi @ang-zeyu! Hmm that is odd. Since I made the change I have not observed this behaviour. May I know which browser you are using so that I may further investigate this issue? Thank you!

Desktop chrome/firefox + mobile chrome on android, it occurs about 50% of the time. Unlike the gif it also occurs on mcq/radio questions (unrelated to a specific type of question).

lhw-1 · 2023-02-27T04:27:04Z

I have tested this on Ubuntu 20.04, on Firefox 109.0.1:

Screencast.from.27-02-2023.12.29.07.webm

And Chromium 110.0.5481.100:

Screencast.from.27-02-2023.12.29.41.webm

Both seem to work fine without flickering.

jovyntls · 2023-02-27T08:26:43Z

Chipping in here on a different environment, I'm getting flickering as well on MacOS with Chrome:

Screen.Recording.2023-02-27.at.4.20.35.PM.mov

And MacOS with FireFox:

Screen.Recording.2023-02-27.at.4.25.14.PM.mov

But it's on and off - can't reproduce it consistently. Also nothing appearing on devtools console 🤔

jovyntls · 2023-02-27T09:05:21Z

@itsyme Try setting throttling using Chrome dev tools (open a mobile view and adjust the throttling option)? I can reproduce the flickering on this branch much more easily when I set throttling to "low-end mobile"

Screen.Recording.2023-02-27.at.5.02.57.PM.mov

itsyme · 2023-02-27T09:18:26Z

Hi @lhw-1 @jovyntls! Thanks for testing it out! My guess is that the previous question's leave transition finishes slightly before the next question's enter transition finishes, leading to the div becoming height 0 for a brief moment. I will try to fix this else I will explore a different approach!

itsyme · 2023-02-27T17:10:46Z

I managed to solve the issue by changing my approach. However as a side effect of this, gotoNextQuestion in Quiz.vue had to be refactored into two functions, gotoNextQuestion and showNextQuestion, in order for the hiding of the current question and the showing of the next question to be called in two separate places. Also, the animation duration was shortened in order to keep a similar effect to the previous animation, since the next question now waits for the previous question to fully leave before starting its animation.

ang-zeyu · 2023-03-11T05:11:26Z

@itsyme Sorry for the late response.

This UX is completely fine to me, I don't see any flickering anymore as well.

My guess is that the previous question's leave transition finishes slightly before the next question's enter transition finishes, leading to the div becoming height 0 for a brief moment. I will try to fix this else I will explore a different approach!

If you'd still like to experiment with the original UX outside of this PR,

If I recall previously you were trying to:

Introduce absolute positioning before the element is inserted.
Remove the absolute positioning after the transition has finished

I think there's a few other options:

Let the quiz expand/shrink to the next card's height just as the "previous card's transition ends".
To be exact this can be either:
- previous card's transition ends - it's still in the DOM so may not be the best option
- previous card leaves the DOM - worth a try if you can sync the removal of position: absolute on the next card to this.
- the next card's transition ends - I believe were going for this previously, it is difficult since the previous card leaving the DOM is not synced to this.
A much easier option (I think it would be better UX wise as well) is to immediately let the quiz expand/shrink to the next card's height just as the user clicks "next". Because you can sync position: absolute on the previous card and the introduction of the next card right at the start.
Make all child cards position:absolute inside the quiz, manage the height at the <quiz> level through the transition.

ang-zeyu · 2023-03-11T05:19:57Z

packages/vue-components/src/__tests__/Quiz.spec.js

+// Function to delay test until question is rendered
+function timeoutRenderQuestion() {
+  // eslint-disable-next-line no-promise-executor-return
+  return new Promise(resolve => setTimeout(resolve, 350));


Is there no way to work around using a timeout?

Something like waitForSelector(...).toGoMissing() (imaginary).
Or creating your own simple transition stub.

There are 17 calls here so resolving this should help improve the speed of tests a fair bit. Unless the runner is parallel, I can't remember..

But don't spend too much time on it, 6s to me is still an ok degradation for now. (just create a follow up issue if we can't resolve this).

@jovyntls could you help to merge in this MR if it looks good to you? 🙇

jovyntls

LGTM! Nice fix @itsyme, let me know if you'll be adding the tests now or opening a future issue for it - will merge if it's the latter

itsyme · 2023-03-13T16:11:04Z

LGTM! Nice fix @itsyme, let me know if you'll be adding the tests now or opening a future issue for it - will merge if it's the latter

I will open a new issue for this, thank you @jovyntls!

itsyme added 5 commits February 23, 2023 11:19

Add timeout before moving to next question

440ffec

Add timeout to Quiz tests

8119627

Update timeout value

9d46f9f

Fix restarting quiz

2109827

Update tests

2b13942

yucheng11122017 reviewed Feb 23, 2023

View reviewed changes

packages/vue-components/src/questions/Quiz.vue Outdated Show resolved Hide resolved

packages/vue-components/src/__tests__/Quiz.spec.js Outdated Show resolved Hide resolved

Fix nits

e21b19b

ang-zeyu reviewed Feb 23, 2023

View reviewed changes

itsyme added 3 commits February 24, 2023 12:54

Remove timeout

9f91a27

Update animation

4687ffe

Update tests

41bf746

Merge branch 'master' into quiz-expanding

972fde5

itsyme added 4 commits February 27, 2023 22:51

Update changing questions in quiz

1a64885

Update animation duration and tests

291da37

Merge branch 'master' into quiz-expanding

c11ed79

Revert translateY

3c339b3

itsyme requested a review from ang-zeyu March 5, 2023 09:55

ang-zeyu reviewed Mar 11, 2023

View reviewed changes

ang-zeyu approved these changes Mar 11, 2023

View reviewed changes

jovyntls added this to the v4.1.1 milestone Mar 13, 2023

jovyntls approved these changes Mar 13, 2023

View reviewed changes

Merge branch 'master' into quiz-expanding

42f2c07

itsyme mentioned this pull request Mar 13, 2023

Add transition stub to optimise testing speed in #2209

Open

jovyntls merged commit 33cd541 into MarkBind:master Mar 14, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Quiz expanding between questions #2184

Fix Quiz expanding between questions #2184

itsyme commented Feb 23, 2023 •

edited

yucheng11122017 left a comment

itsyme commented Feb 23, 2023

itsyme commented Feb 23, 2023

ang-zeyu left a comment

ang-zeyu Feb 23, 2023

itsyme commented Feb 24, 2023

itsyme commented Feb 24, 2023 •

edited

ang-zeyu commented Feb 25, 2023

itsyme commented Feb 25, 2023

ang-zeyu commented Feb 25, 2023

lhw-1 commented Feb 27, 2023 •

edited

jovyntls commented Feb 27, 2023 •

edited

jovyntls commented Feb 27, 2023 •

edited

itsyme commented Feb 27, 2023

itsyme commented Feb 27, 2023

ang-zeyu commented Mar 11, 2023

ang-zeyu Mar 11, 2023

ang-zeyu Mar 11, 2023

jovyntls left a comment

itsyme commented Mar 13, 2023 •

edited

Fix Quiz expanding between questions #2184

Fix Quiz expanding between questions #2184

Conversation

itsyme commented Feb 23, 2023 • edited

yucheng11122017 left a comment

Choose a reason for hiding this comment

itsyme commented Feb 23, 2023

itsyme commented Feb 23, 2023

ang-zeyu left a comment

Choose a reason for hiding this comment

ang-zeyu Feb 23, 2023

Choose a reason for hiding this comment

itsyme commented Feb 24, 2023

itsyme commented Feb 24, 2023 • edited

ang-zeyu commented Feb 25, 2023

itsyme commented Feb 25, 2023

ang-zeyu commented Feb 25, 2023

lhw-1 commented Feb 27, 2023 • edited

jovyntls commented Feb 27, 2023 • edited

jovyntls commented Feb 27, 2023 • edited

itsyme commented Feb 27, 2023

itsyme commented Feb 27, 2023

ang-zeyu commented Mar 11, 2023

ang-zeyu Mar 11, 2023

Choose a reason for hiding this comment

ang-zeyu Mar 11, 2023

Choose a reason for hiding this comment

jovyntls left a comment

Choose a reason for hiding this comment

itsyme commented Mar 13, 2023 • edited

itsyme commented Feb 23, 2023 •

edited

itsyme commented Feb 24, 2023 •

edited

lhw-1 commented Feb 27, 2023 •

edited

jovyntls commented Feb 27, 2023 •

edited

jovyntls commented Feb 27, 2023 •

edited

itsyme commented Mar 13, 2023 •

edited