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] Span GestureRecognizers #2173

Merged
merged 32 commits into from May 7, 2018

Conversation

Projects
@adamped
Copy link
Contributor

adamped commented Mar 23, 2018

Description of Change

fixes #1701

This allows Gesture Recognizers to be added to the Span in a Label. This implementation also provides the basis for other controls to also add Gesture Recognizers to their own child elements, such as a potential image map.

Gesture Recognizers were chosen over a command, as this allows the ability for this solution to expand to include things such as long press on text.

<Label>
     <Label.FormattedText>
          <FormattedString>
              <Span Text="Span1" />
              <Span Text="Span2 (Tap me)" TextColor="Blue">
                  <Span.GestureRecognizers>
                         <TapGestureRecognizer Command="{Binding TapCommand}" />
                   </Span.GestureRecognizers>
              </Span>
              <Span Text="Span3"/>
          </FormattedString>
     </Label.FormattedText>
</Label>

Support for

  • Android
  • iOS / MacOS
  • UWP / WPF

Bugs Fixed

None

API Changes

List all API changes here (or just put None), example:

Added:

  • [Span] GestureRecognizers
  • [IGestureController] GetChildElements
  • [IGestureController] CompositeGestureRecognizers
  • (Internal)[ISpatialElement] Regions

Changed:

  • Span inheritance = Span -> GestureElement -> Element

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description) - Coming soon
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

@samhouts samhouts added this to Ready in v3.1.0 via automation Mar 23, 2018

@samhouts samhouts moved this from Ready to In Review in v3.1.0 Mar 23, 2018

else if (defaultFont != Font.Default)
spannable.SetSpan(new FontSpan(defaultFont, view), start, end, SpanTypes.InclusiveInclusive);
else
spannable.SetSpan(new FontSpan(defaultFont, view), start, end, SpanTypes.InclusiveInclusive);

This comment has been minimized.

@jerone

jerone Mar 24, 2018

Trailing tabs.

@samhouts

This comment has been minimized.

Copy link
Member

samhouts commented Mar 26, 2018

@adamped Would you mind rebasing on master? We've recently removed the docs step that is causing the build to fail. Thanks!

@adamped adamped force-pushed the XamarinFormsCommunity:feature/1701-commandable-span branch from 87cd4cd to d8caa44 Mar 26, 2018

@rmarinho rmarinho requested review from jassmith and hartez Apr 2, 2018

@hartez

hartez approved these changes Apr 3, 2018

Copy link
Member

hartez left a comment

I have some suggestions, but nothing which absolutely has to be changed.

namespace Xamarin.Forms.Internals
{
public interface ISpatialElement
{

This comment has been minimized.

@hartez

hartez Apr 3, 2018

Member

Spaces -> Tabs

@@ -169,6 +187,83 @@ void OnFormattedTextChanged(object sender, PropertyChangedEventArgs e)
OnPropertyChanged("FormattedText");
}

void Span_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e)

This comment has been minimized.

@hartez

hartez Apr 3, 2018

Member

I'm not going to reject the PR over it or anything, but for readability/maintainability, I'd really like to see theses cases extracted into separate methods.

This comment has been minimized.

@adamped

adamped Apr 6, 2018

Author Contributor

Done

break;
case NotifyCollectionChangedAction.Reset:
foreach (IElement item in _gestureRecognizers.OfType<IElement>())
item.Parent = this;
foreach (IElement item in ((IGestureController)this).CompositeGestureRecognizers.OfType<IElement>())

This comment has been minimized.

@hartez

hartez Apr 3, 2018

Member

Just a suggestion - for readability, I'd add a property
protected internal IGestureController GestureController => (IGestureContoller)this;

<ErrorReport>prompt</ErrorReport>
<WarningLevel>4</WarningLevel>
<LangVersion>6</LangVersion>
<LangVersion>7</LangVersion>

This comment has been minimized.

@hartez

hartez Apr 3, 2018

Member

Was there a reason WPF was limited to C# 6 and lower?

This comment has been minimized.

@adamped

adamped Apr 6, 2018

Author Contributor

Not that I could see.

I speculate it was due to the fact that the XF source code used to be restricted to C#6 when the WPF project was being built. But now that restriction has been removed :)

#endif

if (recognizer is ChildGestureRecognizer childRecognizer)
{
#if !__MOBILE__

This comment has been minimized.

@hartez

hartez Apr 3, 2018

Member

Again, not a blocker, but it'd be nice to have this split into separate methods to make it easier to read and maintain.

This comment has been minimized.

@adamped

adamped Apr 6, 2018

Author Contributor

Done

@adamped

This comment has been minimized.

Copy link
Contributor Author

adamped commented Apr 6, 2018

@hartez - made changes as per recommendations

@adamped adamped force-pushed the XamarinFormsCommunity:feature/1701-commandable-span branch from 034fa3a to 7b9c29e Apr 6, 2018

@samhouts samhouts added this to In Progress in Enhancements Apr 13, 2018

@rmarinho rmarinho merged commit 500a8cd into xamarin:master May 7, 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) PR-2173 - (1642006) succeeded
Details
VSTS: Xamarin Forms OSX PR-2173 - (1642010) succeeded
Details
VSTS: Xamarin Forms Windows VS2017 PR-2173 - (1642007) 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 Review to Done May 7, 2018

Enhancements automation moved this from In Progress to Done May 7, 2018

@ahmedalejo

This comment has been minimized.

Copy link

ahmedalejo commented May 7, 2018

@adamped great work!

@samhouts samhouts removed this from Done in v3.1.0 May 7, 2018

@adamped adamped referenced this pull request May 19, 2018

Merged

[All] Bindable span #1850

4 of 4 tasks complete

@samhouts samhouts added this to the 3.2.0 milestone Jun 7, 2018

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

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

@samhouts samhouts modified the milestone: 3.2.0 Sep 12, 2018

@samhouts samhouts added the approved label Sep 12, 2018

@mnknedim

This comment has been minimized.

Copy link

mnknedim commented Sep 16, 2018

@adamped I have run successfully, but the Command method didn't work more than one. So i have research and i found this comment that is "...You can add a GestureRecogizer to the Label that is displaying the FormattedString but there's no easy way to know where in the string the tap occurred."?

@PureWeen

This comment has been minimized.

Copy link
Contributor

PureWeen commented Sep 16, 2018

@mnknedim a number of changes were made to this. Can you test out 3.2 pre3 and if you are still having issues open up a new issue?

@mnknedim

This comment has been minimized.

Copy link

mnknedim commented Sep 17, 2018

@PureWeen Yes, after update of 3.2 pre3 it has run successfully. Thank u so much 👍

@adrianknight89

This comment has been minimized.

Copy link
Contributor

adrianknight89 commented Sep 23, 2018

I tried using this in 3.3.0.840541-pre1, but tap commands were not being hit for me.

@PureWeen

This comment has been minimized.

Copy link
Contributor

PureWeen commented Sep 24, 2018

@adrianknight89 if you have more context that'd be helpful.

David's Little Play things sample uses the binding with 3.3.0 pre1 and it worked fine for me their
https://github.com/davidortinau/TheLittleThingsPlayground

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.