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

[All] Bindable span #1850

Merged
merged 11 commits into from Mar 8, 2018

Conversation

Projects
10 participants
@adamped
Copy link
Contributor

adamped commented Feb 9, 2018

Description of Change

This allows Text to be Bindable on a Span.
It also added the Style property to the Span, to set the Font Elements.

fixes #1340

Bugs Fixed

N/A

API Changes

Added:

  • TextColor { get; set; } // Bindable Property

Marked Obsolete:

  • ForegroundColor { get; set; }
  • ForegroundColor now just updates TextColor

Behavioral Changes

N/A

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

@rmarinho rmarinho requested review from jassmith and StephaneDelcroix Feb 9, 2018

@samhouts samhouts changed the title Feature/1340 bindable span [All] Bindable span Feb 9, 2018

@samhouts samhouts added this to Ready in v3.1.0 via automation Feb 9, 2018

@samhouts samhouts moved this from Ready to In Review in v3.1.0 Feb 9, 2018

@adamped adamped referenced this pull request Feb 10, 2018

Closed

Feature/1340 bindable span #4

3 of 4 tasks complete
@@ -5,163 +5,160 @@

namespace Xamarin.Forms
{
public partial class VisualElement
sealed class MergedStyle : IStyle

This comment has been minimized.

@adamped

adamped Feb 10, 2018

Author Contributor

All I did here was remove the public partial class VisualElement line.

Didn't make any formatting changes, Github just thinks its all different.

{
var formattedString = (FormattedString)newvalue;
formattedString.PropertyChanged += ((Label)bindable).OnFormattedTextChanged;
SetInheritedBindingContext(formattedString, bindable.BindingContext);

This comment has been minimized.

@adamped

adamped Feb 10, 2018

Author Contributor

Changes the BindingContext when it is updated on the Label.

{
base.OnBindingContextChanged();
for (int i = 0; i < Spans.Count; i++)
SetInheritedBindingContext(Spans[i], BindingContext);

This comment has been minimized.

@adamped

adamped Feb 10, 2018

Author Contributor

Sets BindingContext of Spans when the FormattedText BindingContext changes

formattedString.Spans.Add (new Span { BackgroundColor = Color.Red, ForegroundColor = Color.Olive, Text = "Span 1 " });
formattedString.Spans.Add (new Span { BackgroundColor = Color.Black, ForegroundColor = Color.White, Text = "Span 2 " });
formattedString.Spans.Add (new Span { BackgroundColor = Color.Pink, ForegroundColor = Color.Purple, Text = "Span 3" });
formattedString.Spans.Add (new Span { BackgroundColor = Color.Red, TextColor = Color.Olive, Text = "Span 1 " });

This comment has been minimized.

@adamped

adamped Feb 10, 2018

Author Contributor

These are changed because the ForegroundColor is now obsolete, as per Stephane's request.

set { _fontElement.SetValue(FontElement.FontProperty, value); }
public static readonly BindableProperty TextColorProperty = TextElement.TextColorProperty;

public Color TextColor

This comment has been minimized.

@adamped

adamped Feb 10, 2018

Author Contributor

TextColor property now used, to align with rest of XF framework, from ITextElement interface.

@samhouts samhouts added this to In Progress in Enhancements Feb 12, 2018

@adamped adamped referenced this pull request Feb 14, 2018

Open

Feature/1701 Span GestureRecognizers #8

0 of 4 tasks complete
@jassmith
Copy link
Contributor

jassmith left a comment

Looks pretty good, there are plenty of places where double GetValue calls occur but those were there in the past too. If you want to go the extra mile and clean those up, great, otherwise its nowhere near a blocker.

@jassmith

This comment has been minimized.

Copy link
Contributor

jassmith commented Feb 16, 2018

Im wondering if it might make sense to have all the bindings in here be OneTime by default. I doubt that generally speaking spans need dynamic binding and OneTime bindings are much cheaper. We have an opportunity here to make things fast by default in this area at least, even if we cant change it elsewhere.

@StephaneDelcroix
Copy link
Member

StephaneDelcroix left a comment

It looks good, but I think that, as the Parent/Children concept is missing, you won't be able to apply css styles, dynamic resources, ...

I can look at that when I have a few minutes, but in the mean time, do not merge this PR

@jassmith

This comment has been minimized.

Copy link
Contributor

jassmith commented Feb 16, 2018

Thats a really good point stephane. At the bare minimum the Parent will need to be set on the BindableSpan.

@adamped

This comment has been minimized.

Copy link
Contributor Author

adamped commented Feb 17, 2018

@StephaneDelcroix - I set the parent going down. The FormattedText was changed to inherit from Element, then parent set, and Parent set on all Spans.

@jassmith - I assume the GetValue multiple calls was the Text property is called a lot of times, I made minor changes to reduce the number of calls. Down to 1 on iOS and UWP. Down to 2 from 4 on Android.

As for OneTime bindings, I didn't know they existed. How would you implement them?

@jassmith

This comment has been minimized.

Copy link
Contributor

jassmith commented Feb 17, 2018

Just set the default binding mode on the BindableProperty to be OneTime instead of not specifying it (which defaults to OneWay).

@adamped adamped force-pushed the XamarinFormsCommunity:feature/1340-bindable-span branch from f5d8a4c to 5b9546d Feb 17, 2018

@adamped

This comment has been minimized.

Copy link
Contributor Author

adamped commented Feb 17, 2018

@jassmith - ok, added them. BindingMode.OneTime was only added 10 days ago, so I needed to rebase. Didn't know OneTime binding was available.

@jassmith

This comment has been minimized.

Copy link
Contributor

jassmith commented Feb 17, 2018

Yeah its new :)

@@ -66,7 +66,7 @@ public sealed class BindableProperty
throw new ArgumentNullException("declaringType");

// don't use Enum.IsDefined as its redonkulously expensive for what it does
if (defaultBindingMode != BindingMode.Default && defaultBindingMode != BindingMode.OneWay && defaultBindingMode != BindingMode.OneWayToSource && defaultBindingMode != BindingMode.TwoWay)
if (defaultBindingMode != BindingMode.Default && defaultBindingMode != BindingMode.OneWay && defaultBindingMode != BindingMode.OneWayToSource && defaultBindingMode != BindingMode.TwoWay && defaultBindingMode != BindingMode.OneTime)

This comment has been minimized.

@jassmith

jassmith Feb 18, 2018

Contributor

Woops

@smartprogrammer93

This comment has been minimized.

Copy link

smartprogrammer93 commented Mar 4, 2018

Any Updates on this?
It's really needed

Thank you @adamped for your work

@jassmith

This comment has been minimized.

Copy link
Contributor

jassmith commented Mar 6, 2018

build

}

[Obsolete("Foreground is obsolete as of version 2.6.0. Please use the TextColor property instead.")]

This comment has been minimized.

@samhouts

samhouts Mar 6, 2018

Member

We'll want to change this (and probably others in the repo) to 3.0.0 at some point).

@@ -56,9 +56,9 @@ private static string GenerateMarkupText(Span span)
}

// ForegroundColor =>

This comment has been minimized.

@samhouts

samhouts Mar 6, 2018

Member

obsolete comment

@rmarinho

This comment has been minimized.

Copy link
Member

rmarinho commented Mar 6, 2018

Build is failing related to docs issues

@adamped

This comment has been minimized.

Copy link
Contributor Author

adamped commented Mar 7, 2018

@rmarinho @jassmith - I updated the docs. Hopefully it works this time.

@rmarinho

This comment has been minimized.

Copy link
Member

rmarinho commented Mar 7, 2018

@adamped passed docs! thanks

@jassmith

This comment has been minimized.

Copy link
Contributor

jassmith commented Mar 7, 2018

Good I want to get this in

@alanag13

This comment has been minimized.

Copy link
Contributor

alanag13 commented Mar 7, 2018

@jassmith me too, underlines are a big deal

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

StephaneDelcroix commented Mar 8, 2018

This is unlikely styleable using either Style or CSS as FromattedStrings and Spans aren't VisualElement.

@adamped: you don't have to act on this comment, I need to discuss how to fix this with @jassmith

@adamped

This comment has been minimized.

Copy link
Contributor Author

adamped commented Mar 8, 2018

@StephaneDelcroix - Styles work, I even have a unit test for it. Haven't tested on XAML with a Style though, but in code it works.

CSS I haven't tried. Haven't tested merged styles either.

@jassmith

This comment has been minimized.

Copy link
Contributor

jassmith commented Mar 8, 2018

In she goes! Congratulations adam, this is a major feature!

@jassmith jassmith merged commit e42bd62 into xamarin:master Mar 8, 2018

13 checks passed

VSTS: Android API19 Validation Fast Renderers UITests Finished
Details
VSTS: Android API19 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API23 Validation Fast Renderers UITests Finished
Details
VSTS: Android API23 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API25 Validation Fast Renderers UITests Finished
Details
VSTS: Android API25 Validation Legacy Renderers UITests Finished
Details
VSTS: Xamarin Forms (PR Builds) Succeeded PR process
Details
VSTS: Xamarin Forms OSX PR-1850 - (1450829) succeeded
Details
VSTS: Xamarin Forms Windows VS2017 PR-1850 - (1450824) succeeded
Details
VSTS: iOS10 Validation UITests Finished
Details
VSTS: iOS11 Validation UITests Finished
Details
VSTS: iOS9 Validation UITests Finished
Details
license/cla All CLA requirements met.
Details

v3.1.0 automation moved this from In Progress to Done Mar 8, 2018

Enhancements automation moved this from In Progress to Done Mar 8, 2018

@adamped

This comment has been minimized.

Copy link
Contributor Author

adamped commented Mar 8, 2018

Awesome, thanks @jassmith

Will forge ahead with tappable spans now, have slowly been working on them, will be ready for another review soon.

@jassmith

This comment has been minimized.

Copy link
Contributor

jassmith commented Mar 9, 2018

excellent

@findus

This comment has been minimized.

Copy link

findus commented Apr 23, 2018

I love this feature! When will this come to nuget?

@smartprogrammer93

This comment has been minimized.

Copy link

smartprogrammer93 commented Apr 23, 2018

@findus

You can get it now on the nightly build.

From the xamarin nightly repository.

@samhouts samhouts added this to the 3.1.0 milestone May 5, 2018

@VincentH-Net

This comment has been minimized.

Copy link

VincentH-Net commented May 19, 2018

@adamped This is great. I'm really interested in tappable spans, do you still plan on implementing that? Eta? Thx!

@adamped

This comment has been minimized.

Copy link
Contributor Author

adamped commented May 19, 2018

@VincentH-Net - Its already implemented - #2173

@adamped adamped deleted the XamarinFormsCommunity:feature/1340-bindable-span branch May 19, 2018

@rzihe

This comment has been minimized.

Copy link

rzihe commented May 24, 2018

Really excited about this feature. Installed 3.1.0 pre release to test this out but I keep getting the following error: System.MissingFieldException: Field 'Xamarin.Forms.Span.TextProperty' not found.

My code is as followed:
<Label FontSize="Small" TextColor="Gray" > <Label.FormattedText> <FormattedString> <Span Text="{Binding User.Firstname}" FontAttributes="Bold" /> <Span Text="{Binding ResponseIsMatch}" /> <Span Text="{Binding Option.Title}" FontAttributes="Bold"/> </FormattedString> </Label.FormattedText> </Label>
what am I missing?

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.