Skip to content

settimeout based scrolltobottom replaced with nexttick#295

Merged
jspaaks merged 10 commits into
mainfrom
288-nexttick
Oct 19, 2021
Merged

settimeout based scrolltobottom replaced with nexttick#295
jspaaks merged 10 commits into
mainfrom
288-nexttick

Conversation

@jspaaks

@jspaaks jspaaks commented Oct 18, 2021

Copy link
Copy Markdown
Member

Pull request details

List of related issues or pull requests

Refs: #288

Describe the changes made in this pull request

This PR replaces the setTimeout based scroll-to-bottom behavior with nextTick based behavior. Also in this PR:

  • I made the return object for setup() smaller for components ScreenKeywords, ScreenAuthors, and ScreenIdentifiers by moving the function defintions to separate functions
  • I changed to using the fat arrow syntax for the methods in aforementioned setup()s

Instructions to review the pull request

cd $(mktemp -d --tmpdir cffinit-pr.XXXXXX)
git clone https://github.com/citation-file-format/cffinit .
git checkout <this branch>
npm clean-install
npm run dev

go to localhost:8080, see if the app works correctly. Particularly, add authors, keywords or identifiers to the point where the list is longer than what fits the page. then scroll up, then click add author/keyword/identifier. The list should scroll down smoothly to the new, emtpy item at the bottom of the list.

npm run lint
npm run test:unit:ci

@jspaaks jspaaks marked this pull request as ready for review October 18, 2021 10:38
@abelsiqueira abelsiqueira self-requested a review October 19, 2021 08:41

@abelsiqueira abelsiqueira left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the PR, great work.
I noticed that the screen moves down as well as the inner box (when it's zoomed in), so I made a suggestion I found in [1].
I noticed the scrollToBottom function is the same for all screens, can it be factored out for reuse?

[1] https://stackoverflow.com/questions/11039885/scrollintoview-causing-the-whole-page-to-move/11041376

Comment thread src/components/ScreenAuthors.vue Outdated
scrollToBottom()
}
const scrollToBottom = () => {
document.getElementsByClassName('bottom')[0].scrollIntoView({ behavior: 'smooth' })

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
document.getElementsByClassName('bottom')[0].scrollIntoView({ behavior: 'smooth' })
document.getElementsByClassName('bottom')[0].scrollIntoView({ behavior: 'smooth', block: 'nearest' })

Comment thread src/components/ScreenIdentifiers.vue Outdated
editingId.value = -1
}
const scrollToBottom = () => {
document.getElementsByClassName('bottom')[0].scrollIntoView({ behavior: 'smooth' })

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
document.getElementsByClassName('bottom')[0].scrollIntoView({ behavior: 'smooth' })
document.getElementsByClassName('bottom')[0].scrollIntoView({ behavior: 'smooth', block: 'nearest' })

Comment thread src/components/ScreenKeywords.vue Outdated

function addKeyword () {
const scrollToBottom = () => {
document.getElementsByClassName('bottom')[0].scrollIntoView({ behavior: 'smooth' })

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
document.getElementsByClassName('bottom')[0].scrollIntoView({ behavior: 'smooth' })
document.getElementsByClassName('bottom')[0].scrollIntoView({ behavior: 'smooth', block: 'nearest' })

@jspaaks

jspaaks commented Oct 19, 2021

Copy link
Copy Markdown
Member Author

Thanks for the review, and for the useful link. I've refactored scrollToBottom to be a separate function that is now imported in 3 places. I went with block: end and inline: end, which should result in the same behavior as nearest, due to the fact that you can only be above the target element.

@jspaaks jspaaks requested a review from abelsiqueira October 19, 2021 11:29

@abelsiqueira abelsiqueira left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The block: 'end' option moves it up if your app is scrolled down. Should affect different devices as well as people who zoom in a lot. I don't know how it works, but block: 'nearest' seems to fix it.
Reproduce: On a fresh app, zoom to ~125%, scroll down, add a new author.

@jspaaks

jspaaks commented Oct 19, 2021

Copy link
Copy Markdown
Member Author

OK, made changes as requested

@jspaaks jspaaks requested a review from abelsiqueira October 19, 2021 12:03

@abelsiqueira abelsiqueira left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the fixes, it's working great now. 🍋

@jspaaks

jspaaks commented Oct 19, 2021

Copy link
Copy Markdown
Member Author

Yay, good job being persistent. Glad we were able to get it working

@jspaaks jspaaks merged commit 5ad5b48 into main Oct 19, 2021
@jspaaks jspaaks deleted the 288-nexttick branch October 19, 2021 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants