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 scrollbar resetting position on save #16261

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

radu-cernatescu
Copy link
Contributor

Summary of the Pull Request

This PR fixes Issue #11875 by introducing a ScrollViewer and some logic for the scrollbar.

Detailed Description of the Pull Request / Additional comment

The ScrollViewer prevents the scrollbar from scrolling to the top whenever "Save" is clicked in the Settings. In addition, the scrollbar is scrolled to the top of the page whenever navigating to another page within Settings. The scrollbar will not reset if attempting to navigate to the same page that is already navigated to.

Validation Steps Performed

Manual testing of the Settings by building the Terminal app.

PR Checklist

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Settings UI Anything specific to the SUI Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Nov 3, 2023
@radu-cernatescu

This comment was marked as resolved.

@radu-cernatescu radu-cernatescu force-pushed the settings-fix branch 3 times, most recently from 96ab360 to 9845623 Compare November 8, 2023 22:39
@zadjii-msft
Copy link
Member

Can we get rid of all the other <ScrollViewer>'s now too?

@zadjii-msft
Copy link
Member

Actually hold up, I'm pretty sure that this will result in every page having two scrollbars when "Always show scrollbars" is on in the OS settings:
image

@radu-cernatescu radu-cernatescu force-pushed the settings-fix branch 2 times, most recently from 95c427d to f3e7ce0 Compare November 21, 2023 05:53
@DHowett
Copy link
Member

DHowett commented Nov 21, 2023

This feels a lot better! Thanks for getting rid of the inner ScrollViewers!

There's one issue I noticed; perhaps you have an idea how to fix it.

If you open a profile page, and then navigate to "Advanced" (or another sub-page), it remembers the scroll position from the previous page. It results in the list of options being cut off at the top.

@radu-cernatescu
Copy link
Contributor Author

My apologies with the weird force pushing, I will resolve my branch as soon as possible. As for the sub pages, @DHowett, I have the fix, (just need to scroll up when navigating to them) just got my branch a little messed up.
For some reason, the commit states I edited some lines which I didn't touch. I will investigate and have it properly done. Thank you!

@radu-cernatescu
Copy link
Contributor Author

@DHowett,@zadjii-msft,@lhecker : I have fixed the sub pages as well and tested the application. Everything looks good to me now. Let me know if anything is missing or if there's any feedback! Thanks again everyone.

Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

Neat!

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this!

@DHowett DHowett merged commit 0a4fc9b into microsoft:main Nov 27, 2023
12 of 14 checks passed
@radu-cernatescu radu-cernatescu deleted the settings-fix branch November 27, 2023 20:40
DHowett pushed a commit that referenced this pull request Jan 25, 2024
Fix overlapping disclaimer text in Profiles' Defaults section

In #16261, when we removed ScrollViewer from the subpages in the
settings UI, the main Grid child element order was not preserved and as
a result, the disclaimer text overlapped with the main content on the
page.

To fix that we now apply (the lost) `Grid.Row` property on the parent
StackPanel of the main content.

### Validation Steps Performed
- Disclaimer text does not overlap.

### PR Checklist
- [x] Tests added/passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings UI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Settings tab scroll is lost after every save
4 participants