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

OnPlatform/OnIdiom markup extensions #2608

Closed
kzu opened this issue May 4, 2018 · 9 comments

Comments

Projects
7 participants
@kzu
Copy link
Member

commented May 4, 2018

Description

Sometimes changing a single property value per platform is quite convenient, yet the "full" syntax of OnPlatform is overkill for scalar values in attributes.

For example:

<Label Text="Hello World">
    <Label.FontSize>
        <OnPlatform x:TypeArguments="x:Double">
            <On Platform="iOS" Value="20" />
            <On Platform="Android" Value="24" />
        </OnPlatform>
    </Label.FontSize>
</Label>

vs a markup extension version that could provide:

<Label Text="Hello World" FontSize="{OnPlatform Android=24, iOS=20}" />

Same thing applies for OnIdiom, so I think both would make XAML much more concise.

@pauldipietro pauldipietro added this to New in Triage May 4, 2018

@ghuntley

This comment has been minimized.

Copy link

commented May 4, 2018

This would be legit cool. As long as it is properly documented etc. Definitely helps reduce complexity, verbosity and improves readability.

If we had blend on OSX and it supported Xamarin Forms then I could argue it's not needed but we don't.

@hartez hartez removed this from New in Triage May 4, 2018

@hartez hartez added this to Backlog in Enhancements May 4, 2018

@jamesmontemagno

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

I'm into this!

@ahoefling

This comment has been minimized.

Copy link

commented May 4, 2018

This is a great idea!! It will really simplify the verbosity of the XAML which is a pretty big complaint I hear often about using the OnPlatform feature in XAML

ahoefling added a commit to ahoefling/Xamarin.Forms that referenced this issue May 4, 2018

@ahoefling

This comment has been minimized.

Copy link

commented May 4, 2018

I got this working locally, can someone once over my change before I submit the PR? I wasn't able to test this working from the Xamarin.Forms solution but I was able to create this exact same code inside a Xamarin.Forms project and it worked. See the commit in my fork above

@ahoefling

This comment has been minimized.

Copy link

commented May 5, 2018

While I have this code working as a Custom markup extension I am unable to get it working in the Xamarin.Forms platform. I'm not going to be able to submit a PR with what I have because it doesn't work.

I would love some help to get us over this blocker so we can get a PR submitted to add this new markup extension

@samhouts samhouts added this to In Progress in v3.6.0 May 7, 2018

Enhancements automation moved this from Backlog to Done May 14, 2018

StephaneDelcroix added a commit that referenced this issue May 14, 2018

Implement OnPlatform/OnIdiom markup extensions (#2615)
* Added OnPlatform markup extension supporting iOS/Android/UWP for #2608

* Add Default and Other properties to OnPlatformExtension

This allows setting a default value for unknown platforms, as well
as specify values for arbitrary platforms by using a named parameter
like syntax:

`{OnPlatform Android=15, iOS=10, UWP=12, Default=11, Other=Tizen:20}`

The `Other` supports multiple semi-colon separated values. By using
this format, we can make the string more concise than if we used `=`
which would have to be escaped in quotes. For example:

`{OnPlatform Default=10, Other=Tizen:22;Xbox:20;Switch=25;PlayStation=22}`

Added unit tests that verify all the supported combinations.

* Add OnIdiomExtension

The extension allows the following syntax:

`{OnIdiom Phone=23, Tablet=25, Desktop=26, TV=30, Watch=10, Default=20}`

At least one value or `Default` must be specified. `Default` is returned
whenever the specific idiom was not specified.

* Add missing known platforms and return Default if missing

Add all strings that are provided in `Device`.
Convert individual asserts into test cases for better reporting.
Also, whenever a matching platform value isn't specified, return Default
instead of null.

* Turn OnIdiom asserts into test cases

This makes for easier to read, document and report tests.

* Add missing platforms to null check

* Make Default the content property

* Perform type conversion as expected by XamlC

Leverage the conversion that is used elsewhere, to return a
properly typed object that can be assigned directly to the
property. Update tests with typed values since now we get
integers, rather than strings out of the parser.

* Add Converter/ConverterParameter support

* Message should state that the value must be non-null

* Remove Unsupported idiom since it's not useful to set

You can use Default instead.

* Use new GetService<T> extension method for conciseness

* Don't fail if service provider is null

* Remove Other from OnPlatformExtension

As suggested, this might come back in the future in some other form.

@samhouts samhouts moved this from In Progress to Done in v3.6.0 May 14, 2018

@samhouts samhouts removed this from Done in Enhancements Jun 13, 2018

@samhouts samhouts removed this from Done in v3.6.0 Jun 26, 2018

@samhouts samhouts added this to Done in v3.2.0 Jun 26, 2018

@asafgo

This comment has been minimized.

Copy link

commented Sep 16, 2018

I am using Xamarin.Forms version 3.2.0.809874-pre3 I am facing a problem that with this version I am getting the error "The property 'Default' is set more than once" when using OnPlatform at XAML. Here is my code:

<ContentPage.Padding> <OnPlatform x:TypeArguments="Thickness"> <On Platform="Android">10,5,10,0</On> <On Platform="iOS">10,20,10,0</On> </OnPlatform> </ContentPage.Padding>

@kzu

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2018

@asafgo that's not the markup extension form that this issue refers to. The markup extension format for your page would be:

<ContentPage Padding='{OnPlatform Android="10,5,10,0", iOS="10,20,10,0"}'>

Which is arguably immensely more concise ;)

@AtosNicoS

This comment has been minimized.

Copy link

commented Nov 15, 2018

I try to bind IsVisible to the Visible Property. But only for UWP. How does this work? I cannot get it running with the old layout, nor with the new.

Current solution, enabled for all Platforms:

<StackLayout IsVisible="{Binding Visible}">

Attempt with the old style

<StackLayout>
 <StackLayout.IsVisible>
     <OnPlatform x:TypeArguments="x:Boolean">
           <On Platform="WinPhone" Value="{Binding Visible}"></On>
     </OnPlatform>
</StackLayout.IsVisible>

Attempt with the new stype:

<StackLayout IsVisible="{x:OnPlatform UWP={Binding Visible}}">

Could anyone please also tell me if WinPhone is the correct wording or UWP? Is there also a placeholder for "Anything else"/* (in this case ios and android)?

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

@AtosNicoS both OnPlatform syntax expect values, not bindings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.