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

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

Merged
merged 1 commit into from Nov 16, 2016

Conversation

Projects
None yet
8 participants
@pauldipietro
Member

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

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Oct 20, 2016

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,

Contributor

adrianknight89 commented Oct 20, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Oct 20, 2016

Contributor

After this is merged, #422 should be reviewed.

Contributor

adrianknight89 commented Oct 20, 2016

After this is merged, #422 should be reviewed.

@rmarinho

Also this needs rebase..

Show outdated Hide outdated Xamarin.Forms.Platform.Android/AppCompat/MasterDetailPageRenderer.cs
VerticalOptions = LayoutOptions.EndAndExpand,
Children =
{
new Label { Text = "This should be visible." }

This comment has been minimized.

@rmarinho

rmarinho Oct 25, 2016

Member

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

@rmarinho

rmarinho Oct 25, 2016

Member

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

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Nov 2, 2016

Member

can you rebase @pauldipietro

Member

rmarinho commented Nov 2, 2016

can you rebase @pauldipietro

@pauldipietro

This comment has been minimized.

Show comment
Hide comment
@pauldipietro
Member

pauldipietro commented Nov 2, 2016

@rmarinho Done

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Nov 2, 2016

Contributor

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

Contributor

adrianknight89 commented Nov 2, 2016

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

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Nov 2, 2016

Member

@adrianknight89 yes i tested!

Member

rmarinho commented Nov 2, 2016

@adrianknight89 yes i tested!

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Nov 16, 2016

Member

Needs rebase @pauldipietro

Member

rmarinho commented Nov 16, 2016

Needs rebase @pauldipietro

@StephaneDelcroix StephaneDelcroix merged commit 85b349c into master Nov 16, 2016

@rmarinho rmarinho deleted the fix-bugzilla44476 branch Nov 16, 2016

@Vextil

This comment has been minimized.

Show comment
Hide comment
@Vextil

Vextil 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);
        }
    }

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);
        }
    }

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment