Skip to content
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.

Sign up for GitHub

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

removed scroll on adding new question #4181

Merged
merged 4 commits into from
Aug 1, 2023
Merged

removed scroll on adding new question #4181

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
140490f
removed scroll on adding new question
Jaspreet-singh-1032 Jul 2, 2023
81283df
scrollintoview with block end
Jaspreet-singh-1032 Jul 20, 2023
e047184
scroll to the question card
Jaspreet-singh-1032 Jul 27, 2023
68dbb7f
final cleanup
Jaspreet-singh-1032 Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions ...ion/contentcuration/frontend/channelEdit/components/AssessmentEditor/AssessmentEditor.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
<transition-group name="list-complete" tag="div">
<VCard
v-for="(item, idx) in sortedItems"
ref="questionCardRef"
:key="`question-${item.assessment_id}`"
pa-1
class="elevation-4 list-complete-item"
Expand Down Expand Up @@ -333,6 +334,18 @@
this.$analytics.trackAction('exercise_editor', 'Add', {
eventLabel: 'Question',
});
this.$nextTick(() => {
if (this.$refs['questionCardRef'].length >= 1) {
Copy link
Member

@LianaHarris360 LianaHarris360 Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

const lastQuestionCard = this.$refs['questionCardRef'][
this.$refs['questionCardRef'].length - 1
].$el;
const editorDiv = document.getElementById('editViewId');
editorDiv.scrollTo({
top: lastQuestionCard.offsetTop,
behavior: 'smooth',
});
}
});
},
async deleteItem(itemToDelete) {
if (this.isItemActive(itemToDelete)) {
Expand Down
7 changes: 0 additions & 7 deletions ...entcuration/frontend/channelEdit/components/AssessmentItemEditor/AssessmentItemEditor.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,13 +285,6 @@
if (!this.question) {
this.openQuestion();
}
// Assessments are nested inside of a scrolling panel.
// 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maximum of the response type shown by using block: end or nearest.
This is how it looks.
image

Copy link
Member

@MisRob MisRob Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Get coordinates of the question type element
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Jaspreet-singh-1032 Jaspreet-singh-1032 Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to scroll using the outer div with scrollTo function. I think I can implement a workaround using it 💡

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that sounds tricky. Wonderful you found a workaround, let's see. Let us know if you needed anything.

this.$el.scrollIntoView({ behaviour: 'smooth' });
}
},
methods: {
updateItem(payload) {
Expand Down
2 changes: 1 addition & 1 deletion contentcuration/contentcuration/frontend/channelEdit/components/edit/EditView.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<template>

<VContainer ref="editview" fluid class="pa-0 wrapper" @scroll="scroll">
<VContainer id="editViewId" ref="editview" fluid class="pa-0 wrapper" @scroll="scroll">
<VContainer v-if="!nodeIds.length" fluid>
<VLayout justify-center align-center fill-height>
<VFlex grow class="grey--text text-xs-center title">
Expand Down