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

Settings Page Animation Restarts When Switching To Settings Tab #10632

Closed
ghost opened this issue Jul 12, 2021 · 10 comments · Fixed by #10934
Closed

Settings Page Animation Restarts When Switching To Settings Tab #10632

ghost opened this issue Jul 12, 2021 · 10 comments · Fixed by #10934
Labels
Area-Settings UI Anything specific to the SUI Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@ghost
Copy link

ghost commented Jul 12, 2021

When you have a setting tab already open and switch back and forth between the Settings tab and any other tab, the animation of the page (where the page content appears vertically centered and moves upward - which is a bit of an odd animation itself) will restart again, even though this isn't the first time opening the page.

It might have been intended, but I find it a little.... odd.

Proposed technical implementation details (optional)

A different animation might make this appear less odd. Like fade, or any number of animations.

2021-07-12.09-30-48.mp4
@ghost ghost added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jul 12, 2021
@DHowett
Copy link
Member

DHowett commented Jul 12, 2021

I wonder if this is because of us doing a Navigate to the root content view when the settings page comes visible. Perhaps there's something we can do to bind the root content such that it doesn't animate at all on first / subsequent launches. @carlos-zamora this might be related to the view model stuff we were talking about (since doing the navigation in the "code behind" versus as an intrinsic part of the page state may be to blame).

@DHowett
Copy link
Member

DHowett commented Jul 12, 2021

(Thanks for filing this!)

@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-3 A description (P3) Area-Settings UI Anything specific to the SUI and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Jul 12, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 12, 2021
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Jul 12, 2021
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 12, 2021
@carlos-zamora
Copy link
Member

I wonder if this is because of us doing a Navigate to the root content view when the settings page comes visible. Perhaps there's something we can do to bind the root content such that it doesn't animate at all on first / subsequent launches. @carlos-zamora this might be related to the view model stuff we were talking about (since doing the navigation in the "code behind" versus as an intrinsic part of the page state may be to blame).

I did a trace because I was curious. Looks something like this:

  • TerminalPage::_OpenSettingsUI(): select the settings tab and update content
  • MainPage::SettingsNav_Loaded():
    • if (SettingsNav().SelectedItem() == nullptr) --> false
    • nothing happens. Surprisingly, _Navigate() is not called!

So yeah, looks like XAML is just doing it for us.

Playing with the XAML controls gallery, I agree that perhaps a DrillIn or Suppress may work best. Definitely odd!

@Don-Vito
Copy link
Contributor

@carlos-zamora - do you have a preference between Drill in or Surpress? 😊

@carlos-zamora
Copy link
Member

@carlos-zamora - do you have a preference between Drill in or Surpress? 😊

Let's try DrillIn!

@Don-Vito
Copy link
Contributor

DrillIn

@Don-Vito
Copy link
Contributor

@carlos-zamora - WDYT?

@Don-Vito
Copy link
Contributor

OK. Committing :)

@ghost ghost added the In-PR This issue has a related PR label Aug 12, 2021
@ghost ghost closed this as completed in #10934 Aug 12, 2021
@ghost ghost removed the In-PR This issue has a related PR label Aug 12, 2021
ghost pushed a commit that referenced this issue Aug 12, 2021
## PR Checklist
* [x] Closes #10632
* [x] CLA signed. 
* [ ] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already.
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Aug 12, 2021
DHowett pushed a commit that referenced this issue Aug 25, 2021
## PR Checklist
* [x] Closes #10632
* [x] CLA signed. 
* [ ] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already.
@ghost
Copy link

ghost commented Aug 31, 2021

🎉This issue was addressed in #10934, which has now been successfully released as Windows Terminal Preview v1.10.2383.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Aug 31, 2021

🎉This issue was addressed in #10934, which has now been successfully released as Windows Terminal Preview v1.11.2421.0.:tada:

Handy links:

This issue was closed.
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 Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants