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

Only show back button when changing from collapsed details view #941

Merged
merged 5 commits into from
May 21, 2017

Conversation

skendrot
Copy link
Contributor

Changes to when the MasterDetailsView updates the back button visibility. It will now only update the visibility if in the ViewState is MasterDetailsViewState.Details, or if it is changing from MasterDetailsViewState.Details.
The control also will keep track if the back button was visible before it changes it when going into the Details state. If the MasterDetailsView is going from MasterDetailsViewState.Details, it will set the back button visibility to whatever is was previously.

Fixes ##939 and possibly #906. Tested #906 after fixing this and I cannot reproduce

}
else
else if (previousState == MasterDetailsViewState.Details)
{
// Make sure we show the back button if the stack can navigate back
Copy link

@ghost ghost Feb 15, 2017

Choose a reason for hiding this comment

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

This comment does not accurately describe the code block anymore. Could be updated or removed.

Personally I think some explanation can be helpful here; the same explanation you gave in your PR comment can be very insightful.

SetBackButtonVisibility(_stateGroup.CurrentState);

UpdateViewState();
UpdateView(true);
}

private void OnUnloaded(object sender, RoutedEventArgs e)
Copy link

@ghost ghost Feb 15, 2017

Choose a reason for hiding this comment

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

Shouldn't the OnUnloaded method be extended with a method setting the BackButton to its previous state? Right now, when opening the details view and then navigating to another page, the back button remains visible (also in apps where the back button by default is not visible). If the user navigates back and the viewState is still Details, at that point it can be made visible again.

@deltakosh
Copy link
Contributor

Up:)

@deltakosh deltakosh added this to the v1.4 milestone Mar 8, 2017
@skendrot
Copy link
Contributor Author

skendrot commented Mar 8, 2017

Not sure how I feel about changing on Unload. Other thoughts?

@deltakosh
Copy link
Contributor

AFAIC, I don't think it is a problem.

@skendrot
Copy link
Contributor Author

@deltakosh Don't think it's an issue to remove the backbutton? What happens then when navigating forward? Example:

  1. Your app controls the back button visibility when navigating (Frame.Navigating event)
  2. Page with MasterDetailsView in narrow state
  3. Select an item from Master
  4. Do something in details that triggers navigating to a new page.
  5. App sets backbutton visibility due to page navigation
  6. MasterDetailsView is unloaded and removes backbutton visibility

@deltakosh
Copy link
Contributor

We can provide a Boolean to control this behavior. What are the other options?

@skendrot
Copy link
Contributor Author

What would the proposed property be? ControlBackbuttonVisibilityWhenUnloaded, ResetBackButtonVisibilityWhenUnloaded ?
We can't set back to the previous visibility as was done for this PR because it breaks the above example
We could check if there are items in the backstack as was previously done. But this breaks @aquilanl's original example still

@deltakosh
Copy link
Contributor

Ok gotcha..This is tougher than what I was expecting

@skendrot
Copy link
Contributor Author

I'm good with a property, but what should it be called? It's a very specific property. What should it do? Should it reset, should it do nothing, should it check if there are items in the Frame stack?

@deltakosh
Copy link
Contributor

Hehe! I'm not really good at naming (at least in English) but I would find ControlBackbuttonVisibilityWhenUnloaded clear enough.
When off the unloaded method will do nothing.

@deltakosh deltakosh modified the milestones: v1.5, v1.4 Mar 20, 2017
@deltakosh
Copy link
Contributor

Moving to 1.5 (ping me if you think you can still make it for 1.4)

@nmetulev
Copy link
Contributor

as @deltakosh would say, Up :)

@skendrot
Copy link
Contributor Author

Propose this be merged and a new issue be opened to address any additional issues

@nmetulev
Copy link
Contributor

Opened #1176 to discuss. Merging this.

@nmetulev nmetulev merged commit b18a90c into CommunityToolkit:dev May 21, 2017
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.

4 participants