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

[Enhancement] Add Visual progress behind feature flag #4449

Merged
merged 13 commits into from
Nov 23, 2018
Merged

[Enhancement] Add Visual progress behind feature flag #4449

merged 13 commits into from
Nov 23, 2018

Conversation

PureWeen
Copy link
Contributor

@PureWeen PureWeen commented Nov 16, 2018

Description of Change

Add a Visual property to VisualElement that allows us to swap out different renderers based on a Visual Type property

fixes #4435

This PR also adds a starting set of Material Renderers as the first use case for this behavior

  • Button
  • Entry
  • Frame (CardView)
  • ProgressBar (iOS)

API Changes

Added:

public class VisualElement
{
		public IVisual Visual
		{
			get { return (IVisual)GetValue(VisualProperty); }
			set { SetValue(VisualProperty, value); }
		}
}
public sealed class ExportRendererAttribute : HandlerAttribute
{
		public ExportRendererAttribute(Type handler, Type target) : this(handler, target, null)
		{
		}
}
// Example Usage
[assembly: ExportRenderer(typeof(Xamarin.Forms.Button), typeof(Xamarin.Forms.Platform.iOS.Material.MaterialButtonRenderer), new[] { typeof(VisualRendererMarker.Material) })]
namespace Xamarin.Forms.Platform.iOS.Material
{
	public class MaterialButtonRenderer : ViewRenderer<Button, MButton>

Platforms Affected

  • Core/XAML (all platforms)
  • iOS
  • Android
  • UWP

Before/After Screenshots

There should be no visual changes to standard Default Renderers
Here is the view when using Material Visuals

image

image

Testing Procedure

  • Make sure Visuals are hidden behind the experimental flag correctly
  • Make sure all current renderer behavior has been maintained (lots of changes to registrar) so that this change isn't breaking

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

@PureWeen PureWeen added DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. and removed DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. labels Nov 16, 2018
@hartez hartez requested a review from samhouts November 19, 2018 17:50
Copy link
Member

@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.

Except for the part where you shuffle stuffs that were not misplaced, I think that the Core stuffs are nice.

Xamarin.Forms.Core/Element.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Core/ExperimentalFlags.cs Show resolved Hide resolved
Xamarin.Forms.Core/IPropertyPropagationController.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Core/Items/CollectionView.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Core/Properties/AssemblyInfo.cs Show resolved Hide resolved
Xamarin.Forms.Core/VisualElement.cs Outdated Show resolved Hide resolved
Copy link
Member

@samhouts samhouts left a comment

Choose a reason for hiding this comment

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

No blockers!

Xamarin.Forms.Core.UnitTests/VisualTests.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Core.UnitTests/VisualTests.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Core.UnitTests/VisualTests.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Core/Visual.cs Outdated Show resolved Hide resolved
Copy link
Member

@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 blocker.

@@ -389,5 +390,26 @@ public override object ConvertFromInvariantString(string value)
return new ButtonContentLayout(position, spacing);
}
}


protected override void OnPropertyChanged([CallerMemberName] string propertyName = null)
Copy link
Member

Choose a reason for hiding this comment

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

we don't usually override OnPropertyChanged but hook into the BP propertyChanged:, but this is fine

Xamarin.Forms.Core/Element.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Core/ExperimentalFlags.cs Show resolved Hide resolved
Xamarin.Forms.Core/Properties/AssemblyInfo.cs Show resolved Hide resolved
Xamarin.Forms.Core/Visual.cs Show resolved Hide resolved
Xamarin.Forms.Core/Visual.cs Outdated Show resolved Hide resolved
@rmarinho rmarinho merged commit 94e6621 into master Nov 23, 2018
@samhouts samhouts added roadmap API-change Heads-up to reviewers that this PR may contain an API change labels Nov 29, 2018
@samhouts samhouts added this to the 4.0.0 milestone Dec 4, 2018
@samhouts samhouts modified the milestones: 4.0.0, 3.5.0 Jan 10, 2019
@samhouts samhouts added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Jan 11, 2019
@samhouts samhouts removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a/visual API-change Heads-up to reviewers that this PR may contain an API change p/Android p/iOS 🍎 p/UWP roadmap t/enhancement ➕
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spec: Visual
4 participants