Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[A] Check for parent NavigationPage when setting MDP detail's TopPadding #473

Merged
merged 1 commit into from
Nov 16, 2016

Conversation

pauldipietro
Copy link
Contributor

@pauldipietro pauldipietro commented Oct 19, 2016

Description of Change

Additional top padding was being added onto a MasterDetailPage's detail when inside of a NavigationPage in AppCompat, presumably from the calculations that the NavigationPage itself was already doing. This additional status bar height being set as padding can be avoided by checking to see if the parent is a NavigationPage or not.

Gallery reproduction added, although it's a visual issue.

Bugs Fixed

https://bugzilla.xamarin.com/show_bug.cgi?id=44476

API Changes

None

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

@adrianknight89
Copy link
Contributor

@pauldipietro I think you need to add this check to Master as well if it's in splitscreen mode. In tablets for example, both master and detail could appear side by side.

Add the same check to TopPadding = ((IMasterDetailPageController)newElement).ShouldShowSplitMode ? statusBarHeight : 0,

@adrianknight89
Copy link
Contributor

After this is merged, #422 should be reviewed.

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

Also this needs rebase..

@@ -116,7 +116,7 @@ void IVisualElementRenderer.SetElement(VisualElement element)
{
_detailLayout = new MasterDetailContainer(newElement, false, Context)
{
TopPadding = statusBarHeight,
TopPadding = Element.Parent is NavigationPage ? 0 : statusBarHeight,
Copy link
Member

Choose a reason for hiding this comment

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

So what if Parent is not a Navigation, but something like TabbedPage, and that was as a parent a Navigation? Get me?
Nav -> Tab > MDP -> we don't like this in Forms, but in this case your fix doesn't work, maybe a recursive method to check if we have a Nav above us.

VerticalOptions = LayoutOptions.EndAndExpand,
Children =
{
new Label { Text = "This should be visible." }
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a test to check if this text appears ?

@rmarinho
Copy link
Member

rmarinho commented Nov 2, 2016

can you rebase @pauldipietro

@pauldipietro
Copy link
Contributor Author

@rmarinho Done

@adrianknight89
Copy link
Contributor

@rmarinho Was the master component tested in split screen/view mode?

@rmarinho
Copy link
Member

rmarinho commented Nov 2, 2016

@adrianknight89 yes i tested!

@rmarinho
Copy link
Member

Needs rebase @pauldipietro

@StephaneDelcroix StephaneDelcroix merged commit 85b349c into master Nov 16, 2016
@rmarinho rmarinho deleted the fix-bugzilla44476 branch November 16, 2016 12:50
@Vextil
Copy link

Vextil commented Jan 27, 2017

This does not fix the problem for the Master when in "Split" mode.

If someone needs a fix right now, (as horrible as it may be) just use the following renderer:

public class FixedMasterDetailPageRenderer: MasterDetailPageRenderer
    {
        protected override void OnElementChanged(VisualElement oldElement, VisualElement newElement)
        {
            base.OnElementChanged(oldElement, newElement);

            var detailContainer = GetChildAt(0);
            var masterContainer = GetChildAt(1);

            // Use reflection to modify MasterDetailContainer's (internal class) TopPadding
            var topPadding = detailContainer.GetType().GetProperty("TopPadding");
            topPadding.SetValue(detailContainer, 0);
            topPadding.SetValue(masterContainer, 0);
        }
    }

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants