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 the BackgroundColor to Background update #6372

Merged
merged 9 commits into from
Apr 22, 2022
Merged

Conversation

mattleibow
Copy link
Member

@mattleibow mattleibow commented Apr 21, 2022

Description of Change

The reason setting the BackgroundColor of a Page did not update the Background was due to the "inheritance" of the handlers. The PageHandler effectively inherits from ViewHandler, however, we remap some things in Controls and this caused an issue because we used the wrong type.

The VisualElement.RemapForControls assumed that all VisualElements are Views, and this is not true for the case of Page. However, this is true for the case of IView.

Issues Fixed

@mattleibow mattleibow added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Apr 21, 2022
@mattleibow mattleibow self-assigned this Apr 21, 2022
@mattleibow mattleibow added this to the 6.0.300-rc.3 milestone Apr 21, 2022
@mattleibow
Copy link
Member Author

I see @jsuarezruiz did this, but in another way: #5706

Not sure if changing to IView is good or bad? @PureWeen ?

Comment on lines -9 to +13
public static IPropertyMapper<IView, IViewHandler> ControlsVisualElementMapper = new PropertyMapper<View, IViewHandler>(Element.ControlsElementMapper)
{
[nameof(BackgroundColor)] = MapBackgroundColor,
};
public static IPropertyMapper<IView, IViewHandler> ControlsVisualElementMapper =
new PropertyMapper<IView, IViewHandler>(Element.ControlsElementMapper)
{
[nameof(BackgroundColor)] = MapBackgroundColor,
};
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the actual fix: View -> IView

Copy link
Contributor

@StephaneDelcroix StephaneDelcroix left a comment

Choose a reason for hiding this comment

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

no tests ?

@mattleibow
Copy link
Member Author

Still working on that.

@rachelkang
Copy link
Member

I changed the Page BackgroundColor to Background in the template styles - should we change that back with this fix? Or should either work fine now?

@PureWeen
Copy link
Member

@rachelkang I would change it back. Since setting Background makes BackgroundColor unusable it's unfortunately confusing if we're setting Background in our styles and they go to set BackgroundColor

@mattleibow mattleibow requested a review from a team as a code owner April 22, 2022 16:36
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Blazor related changes look good.

Can't comment on other changes!


public static CommandMapper<IPicker, IContentViewHandler> CommandMapper = new(ViewCommandMapper)
Copy link
Member Author

Choose a reason for hiding this comment

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

oops for the IPicker :(

@pedromcrocha
Copy link

pedromcrocha commented Sep 3, 2022

in https://github.dev/dotnet/maui/blob/main/src/Controls/src/Core/Compatibility/Handlers/VisualElementRenderer.cs
around line 175
where it is
#if IOS
protected virtual void SetBackgroundColor(Color? color)
#else
protected virtual void UpdateBackgroundColor()
#endif
{
if (Element != null)
ViewHandler.MapBackground(this, Element);
}

shouldn't be?

#if IOS
protected virtual void SetBackgroundColor(Color? color)
#else
protected virtual void UpdateBackgroundColor()
#endif
{
if (Element != null)
ViewHandler.MapBackgroundColor(this, Element);
}

@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2023
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.