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

Fix component list scrolling #10937

Merged
merged 3 commits into from
Sep 2, 2024

Conversation

vitvakatu
Copy link
Contributor

Pull Request Description

Fixes #10927

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

@vitvakatu vitvakatu added p-highest Should be completed ASAP CI: No changelog needed Do not require a changelog entry for this PR. --bug Type: bug -gui labels Aug 30, 2024
@vitvakatu vitvakatu self-assigned this Aug 30, 2024
Copy link
Contributor

@Frizi Frizi left a comment

Choose a reason for hiding this comment

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

I'm not sure about this bugfix, the old code here seems to me actually correct. All it does is to select a range of elements to render baesd on scroller viewport, and I don't see any flaw in that logic.

The issue seems to be that the scrolling.scrollPosition can actually lie - the reported "scroll position" is past the scroll height bounds. So my propsed fix would be to instead limit that value, something like this:

const scrolling = useScrolling(() =>
  Math.min(animatedHighlightPosition.value, listContentHeight.value - scrollerSize.value.y),
)

Copy link
Contributor

@Frizi Frizi left a comment

Choose a reason for hiding this comment

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

Sorry didn't mean to approve 🫨

@vitvakatu
Copy link
Contributor Author

Indeed, the proposed fix is easier and works perfectly fine.

@vitvakatu vitvakatu requested a review from Frizi September 2, 2024 09:39
@vitvakatu vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label Sep 2, 2024
@mergify mergify bot merged commit 3a310dc into develop Sep 2, 2024
37 checks passed
@mergify mergify bot deleted the wip/vitvakatu/fix-component-list-scrolling branch September 2, 2024 17:09
@enso-bot
Copy link

enso-bot bot commented Sep 9, 2024

Ilya Bogdanov reports a new STANDUP for the provided date (2024-08-30):

Progress: Debugged and fixed component list scrolling. It should be finished by 2024-08-30.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug -gui CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge p-highest Should be completed ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Moving cursor in the Component Browser Makes things Vanish
2 participants