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

Fixing Scroller.ScrollToTop, alias to a new fyne.ScrollDirection type and align all collections on ScrollToOffset #5477

Merged
merged 5 commits into from
Feb 1, 2025

Conversation

andydotxyz
Copy link
Member

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.
  • Public APIs match existing style and have Since: line.

@coveralls
Copy link

coveralls commented Jan 29, 2025

Coverage Status

coverage: 62.645% (+1.8%) from 60.864%
when pulling bb2297d on andydotxyz:fix/5456
into fba693b on fyne-io:develop.

@andydotxyz andydotxyz requested a review from Jacalz January 30, 2025 22:02
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Perfect. Well done. I just added a question inline :)

scroll.go Outdated
ScrollVerticalOnly
// ScrollNone turns off scrolling for this container.
//
// Since: 2.6
Copy link
Member

Choose a reason for hiding this comment

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

All of these above are new within the fyne package scope. I understand why this was added here but it is slightly confusing 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I get your point but what would be more correct? I can switch to whatever you suggest, but I'm not sure what is right.

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest adding the line to the type only as it is new in this release but I don't know really.

Copy link
Member Author

Choose a reason for hiding this comment

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

That works, done thanks :)

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Splendid 👍

@andydotxyz andydotxyz merged commit 6503f9e into fyne-io:develop Feb 1, 2025
11 checks passed
@andydotxyz andydotxyz deleted the fix/5456 branch February 1, 2025 12:25
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.

3 participants