removed scroll on adding new question #4181
Conversation
MisRob
requested review from
akolson,
LianaHarris360,
ozer550,
AllanOXDi and
thanksameeelian
| // Instead of propagating an event all the way back to | ||
| // the scrolling panel, just use scrollIntoView | ||
| // (supported by most major browsers) | ||
| if (this.$el.scrollIntoView) { |
There was a problem hiding this comment.
Hi @Jaspreet-singh-1032, thank you for the changes here. Although I have some queries, removing this if statement would completely eliminate the smooth animation effect. Instead, we may want to enable scrolling with animation to the "response type" field rather than the "title/question". @MisRob @LianaHarris360 can you guide on the exact behaviour here? Removing the if statement does seem to work altho there won't be any scroll animation.
There was a problem hiding this comment.
Yes, I think we would want to keep the animation scroll. As for enabling scrolling to the "response type" field, the scrollIntoView() method takes in a block argument in addition to behavior, that defines the vertical alignment and the default is set to 'start' which doesn't seem to scroll to the preferred view, so possibly trying 'center' or 'nearest' could be an option.
There was a problem hiding this comment.
The maximum of the response type shown by using block: end or nearest.
This is how it looks.
There was a problem hiding this comment.
It seems to me that it's caused by the secondary bar (with "Details", "Questions", "Related"). Even though the scroll position is correct, the element we scrolled into view is hidden underneath the bar. I think that one way to deal with it might be something like:
- Get coordinates of the question type element
- Scroll window (https://developer.mozilla.org/en-US/docs/Web/API/Window/scrollTo) to those coordinates but with an offset that corresponds to the bar height.
There was a problem hiding this comment.
Thanks, I will try this solution. However, the window.scrollTo function was not working here, but I will research it. It could be due to some CSS.
There was a problem hiding this comment.
Ah, yes, this is a modal actually, so I assume that's the reason. Perhaps Element.scrollTo() https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollTo could be used then.
There was a problem hiding this comment.
The scrollTo did not work on any of the elements. I think this is because there is an outer scrollable div in which all these elements are rendered and the scrollTo only works on the outer parent div and not on the internal elements.
There was a problem hiding this comment.
I was able to scroll using the outer div with scrollTo function. I think I can implement a workaround using it 💡
There was a problem hiding this comment.
Ah, that sounds tricky. Wonderful you found a workaround, let's see. Let us know if you needed anything.
|
@Jaspreet-singh-1032 @MisRob @ozer550 @LianaHarris360 I've manually tested this, its working as expected. Please go through the code once to see if we can simplify it more. Otherwise lets get it merged! @Jaspreet-singh-1032 This was really a very tricky one, you did an excellent job with this, ❤️ I am really impressed! Here's one of the more tricky issue that I think you are an excellent fit to deal with: #4170. Let me know personally on Slack or on dev-community channel if you have any questions (you should have questions!). cc @bjester. |
| @@ -333,6 +334,18 @@ | |||
| this.$analytics.trackAction('exercise_editor', 'Add', { | |||
| eventLabel: 'Question', | |||
| }); | |||
| this.$nextTick(() => { | |||
| if (this.$refs['questionCardRef'].length >= 1) { | |||
There was a problem hiding this comment.
To avoid some redundancy, this.$refs['questionCardRef'] can be defined as a constant before the if statement and reused within.
Also consider using optional chaining (?.) to prevent potential errors from null values/handle them safely, for example:
if (questionCards?.length >= 1)
To echo @vkWeb you did a great job with this workaround!
There was a problem hiding this comment.
The code changes look great to me and it works as intended. Thanks @Jaspreet-singh-1032
|
@LianaHarris360 I've merged this! 🎉 |
|
LGTM as well! |
Summary
fixes #4079
Removed scroll on adding new questions
Screenshots (if applicable)
https://www.loom.com/share/b19535f245364c39899f85d9db974202
Reviewer guidance
How can a reviewer test these changes?
Contributor's Checklist
PR process:
CHANGELOGlabel been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocslabel has been added if this introduces a change that needs to be updated in the user docs?requirements.txtfiles also included in this PRStudio-specifc:
notranslateclass been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages,components, andlayoutsdirectories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarnandpip)