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.

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

Refactoring Scrolling Behaviour (WIP) #22

Closed

Conversation

itsonlyjames
Copy link
Collaborator

@itsonlyjames itsonlyjames commented Aug 12, 2024

Opening this in an attempt to fix a few issues:
Fixes: #21
Fixes: #20

I think we can utilise a bit more of the codeMirror functionality in getting the content position, thus being able to remove the padding top solution that is current.

Also includes a few minor code cleanings.

Needs a fair bit of work, but I think this could be great. I'll also implement a setting for the scroll behavior either "instant" or "smooth".

Keen to see what you think and if it sparks any ideas!

@itsonlyjames itsonlyjames changed the title Refactoring Scrolling Behaviour Refactoring Scrolling Behaviour (WIP) Aug 12, 2024
src/Plugin.ts Show resolved Hide resolved
@zhouhua
Copy link
Owner

zhouhua commented Aug 12, 2024

That's great! I'm not very familiar with CM, so if you can help optimize it, that would be fantastic. Of course, your method will certainly be better than my use of padding-top. Thank you so much for your work, and I'm really looking forward to your optimized version.

@itsonlyjames
Copy link
Collaborator Author

@zhouhua I've made some progress, and have gotten it working where it appends the headers at the correct position, however I'm struggling to get it accurately to scroll back to the position of the header minus the sticky heading element as it changes height along the way—I see why you did it the way you did it.

If you can run and take a look at my code, specifically in the header onclick, maybe your fresh set of eyes might have an idea? At the end of the day, I think it's just timing of calculating the new sticky header height and when to apply that offset, for example, clicking on a header when sticky header has 2 elements, and arriving at a header that had 5 now in the header, how do we best handle this (your previous way makes total sense). However, you'll see there's no document reflow going on anymore which is great, but it's just that last little bit.

No rush, when you get some time.

Side note: if you test out behavior: "smooth" it works great for all headings except the very first one, I'm not sure if it's to do with the debounce checking on scroll or what, but have left it as instant for now—however would love to get smooth working ideally as it's very smooth. Cheers

@itsonlyjames itsonlyjames marked this pull request as draft August 13, 2024 12:20
@zhouhua
Copy link
Owner

zhouhua commented Aug 14, 2024

@itsonlyjames I have observed some interesting phenomena.

  1. The error in position calculation is due to the height of the inline title. When the inline title is enabled, the calculation of whether the title is out of the visible area is inaccurate because it needs to subtract the height of the inline title. However, in this case, the scroll position when clicking the title is relatively accurate. Conversely, when the inline title is disabled, the calculation of whether the title is out of the visible area is accurate, but the scroll position when clicking the title is inaccurate and needs to subtract the height of the inline title (if it is displayed). This is very strange; I believe it might be a bug in Obsidian.

  2. Yes, I believe the debounce is problematic. It seems that the last render isn't being triggered. I'm not sure why this is happening; perhaps Obsidian itself has optimizations for dom event? For now, we could try removing the debounce to ensure correct rendering, and then optimize performance afterward.

@itsonlyjames
Copy link
Collaborator Author

itsonlyjames commented Aug 14, 2024

@zhouhua great insights, I appreciate the knowledge.

  1. Should/could we simply check if the inline title is active and adjust calculations accordingly for now? I can't imagine this would be too difficult. However my main concern is if the cm-scroller element is affected, with top 0 not actually being the top, like you say. In which case, we may need to force a scrollBy -200 to actually get to the top.
    1.1. I would agree with the Obsidian bug, because codeMirror does not have inline titles, and as such, this is Obsidian specific. I am doubtful they would implement a fix in a short period of time howerver, so I think it's on us to implement a solution.
  2. Even without the debounce the issue occurs after my initial testing. It's like the sticky heading gets stuck with its height and doesn't update, running rerenderall in a timeout on the heading element click can force fix the issue, but I believe we can optimise at the root here.

I think extrapolating out some of the logic, into separate functions might be best. Plus for future development, having smaller functions might give us an easier time to debug.

This might take a while, given we're both busy. Are you able to pull this branch from my repo and contribute to this PR also? Or can you pull this PR locally in your repo so you may contribute? I think it is going to take both of our brains to get a proper solid, performant solution in place. I look forward to the challenge!

@zhouhua
Copy link
Owner

zhouhua commented Aug 15, 2024

A quick question: how do we contribute code to the same branch? Do you have any recommended collaboration methods?

@itsonlyjames
Copy link
Collaborator Author

I would suggest branching off and PR to merge back in if it's a big enough change, e.g., a complete refactor. Otherwise we're working on the same thing so I don't mind fixing a couple merge conflicts. If you want to take this branch and create your own within your repo I'd be happy to become a collaborator and work alongside you.

@zhouhua
Copy link
Owner

zhouhua commented Aug 15, 2024

I haven't done much open-source work before, so I lack experience. I've invited you to my repo as a collaborator; I think this will make things more convenient.

@zhouhua
Copy link
Owner

zhouhua commented Aug 15, 2024

I've copied your branch to my repo. I will close this PR, and we can continue this work in #23.

@zhouhua zhouhua closed this Aug 15, 2024
@itsonlyjames itsonlyjames deleted the feature/smoothScroll branch August 19, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants