Implementing Tapped on Images in MarkdownTextBlock #1807

avknaidu · 2018-02-13T13:46:06Z

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build or CI related changes
[ ] Documentation content changes
[ ] Sample app changes
[ ] Other... Please describe:

What is the current behavior?

Images cannot be clicked with current implementation

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
Docs have been added/updated which fit documentation template. (for bug fixes / features)
Sample in sample app has been added / updated (for bug fixes / features)
- Icon has been created (if new sample) following the Thumbnail Style Guide and templates
Tests for the changes have been added (for bug fixes / features) (if applicable)

What is the new behavior?

Images can be tapped and will work as regular Hyperlink giving user option to Handle them in LinkClicked event

Does this PR introduce a breaking change?

[ ] Yes
[x] No

avknaidu · 2018-02-13T13:47:51Z

@skendrot @nmetulev @WilliamABradley Although the Link Clicked on Images also work, I would like to add a Extra Property e.Type which will return if the clicked FrameworkElement is a Link or Image. That way dev can choose what they want to do with the clicked Links. Thoughts?

skendrot · 2018-02-13T16:32:49Z

What's the use case for needing to know if an image was clicked?

avknaidu · 2018-02-13T16:48:35Z

Devs can handle Links and Images in 2 different UI ways. Gives them benefit of building Different UI's for each Link Type if required.

For example, Links can be opened in a web view inside the app and Images can be opened in a Content Dialog with full scale width an height even though in MarkdownTextblock it it set to specific ImageStretch. If this type is not set, Devs need to figure out if the clicked link is a regular link or an image link. We can eliminate that and provide that info from Toolkit Itself. Make their life a little bit more easy.

skendrot · 2018-02-13T17:07:31Z

Aren't images already links?

avknaidu · 2018-02-13T17:15:03Z

They are. When you clicked on the Image, It opened a New Tab. It does the same when you click on a regular link too. With Toolkit returning what is that Link Pertaining to, it will give an extra option for Developer to actually do different things while handling links.

Maybe if we return e.Type as HyperLink and Image instead of Link and Image it might make more sense.

WilliamABradley · 2018-02-14T04:58:02Z

@avknaidu #1650 has now been merged, you will need to ensure that your PR conforms to the new structure.

nmetulev · 2018-02-14T05:00:35Z

I agree, Type seems like a good addition.

skendrot · 2018-02-14T17:02:48Z

sorry, I was misunderstanding the PR. I thought you could already tap an image. That was this PR is doing!

I'm not sure adding a type is all that beneficial. Easy enough to know it's an image based on the uri

nmetulev · 2018-02-14T17:15:05Z

It's easy to know if an uri is an image, but not if that uri is coming from an image or a regular link

avknaidu · 2018-02-16T20:39:13Z

@skendrot @nmetulev let me know if i need to make any more changes.

WilliamABradley · 2018-02-16T21:51:46Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/MarkdownTextBlock.Properties.cs

        /// Holds a list of hyperlinks we are listening to.
        /// </summary>
-        private readonly List<Hyperlink> _listeningHyperlinks = new List<Hyperlink>();
+        private readonly List<object> _listeningHyperlinks = new List<object>();


It would be nice to wrap this into a new class, or tuple or something, for Type safety.

You could change it to UIElement (I think).

Image is derived from UIElement. Pretty sure Hyperlink is not.

Ah, that's right. I'm ok with this being how it is right now unless @WilliamABradley has any concerns

nmetulev · 2018-02-18T22:01:28Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/LinkClickedEventArgs.cs

+        /// <summary>
+        /// Gets the type of link that was tapped.(Image/Link)
+        /// </summary>
+        public string Type { get; }


Could you change this to Enum instead of a string?

nmetulev

Looks good to me. other than the change requested by @WilliamABradley

nmetulev · 2018-02-19T20:43:59Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/MarkdownTextBlock.Properties.cs

        /// Holds a list of hyperlinks we are listening to.
        /// </summary>
-        private readonly List<Hyperlink> _listeningHyperlinks = new List<Hyperlink>();
+        private readonly List<object> _listeningHyperlinks = new List<object>();


You could change it to UIElement (I think).

nmetulev · 2018-02-19T20:47:20Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/LinkReturnType.cs

+// THE CODE OR THE USE OR OTHER DEALINGS IN THE CODE.
+// ******************************************************************
+
+


I think there is an extra space here, that's why the build is failing.

avknaidu · 2018-02-19T22:52:07Z

Do not merge this PR yet. I found an issue with my commit. Fixing it.

skendrot · 2018-02-19T23:43:37Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/MarkdownTextBlock.Methods.cs

+            foreach (object link in _listeningHyperlinks)
            {
-                link.Click -= Hyperlink_Click;
+                if (link is Hyperlink)


change the if statements to reduce casting

if(list is Hyperlink hyperlink) { hyperlink.Click -= Hyperlink_Click; } else if(link in Image image) { image.Tapped -= NewImagelink_Tapped; }

skendrot

I haven't had a chance to test the new feature, but code looks good. @nmetulev have you had a chance to test this?

nmetulev · 2018-02-20T18:16:32Z

I tested it and works as expected.

Keep in mind, this PR does not add support for images to be wrapped in a hyperlink
so this won't work: [![image](http://imagelink)](http://link.com)

I think this is something that should be added next (in a separate PR) ;)

nmetulev · 2018-02-20T20:17:34Z

@WilliamABradley, up to you now

avknaidu · 2018-02-20T21:06:24Z

@nmetulev I already did some research on the option above because it is the most commonly used scenario on Stackoverflow. I made changes to fit that scenario but they do not meet the standard of this toolkit. However when i work on updating my app, I will revisit this scenario and will implement that in a proper way. Probably before 3.0

WilliamABradley · 2018-02-21T06:17:11Z

@avknaidu This throws an exception on Local images in the sample app, you should catch all exceptions for link clicks, since other people could have implemented unsafe link launching like we have in the sample app, and it would be a breaking change.

The issue is on line 523 in Shell.xaml.cs.

avknaidu · 2018-02-21T15:48:23Z

Clicking on Relative Links is an issue even with 2.1.1 release. If anything, this PR will fix that issue in sample app. I am not sure if a bug fix + improvement is considered a breaking change.

If you install the app from the store, enter [Link 1](/Link.html) and click on the Link in MarkdownTextBlock in sample below, It will close the app completely. So if any devs who use Relative Links option on MarkdownTextBlock from 2.1.1 notice this issue, They will already be handling the scenario because there is no way the app works without handling the scenario.

Although the Exception in the sample app for docs you mentioned above is handled with my last Push. If you want me to update the docs, I will be happy to do that.

nmetulev · 2018-02-21T17:31:41Z

I think it's worth updating the docs to add the notice. Relative images didn't work before so don't think it's going to be a big issue.

nmetulev · 2018-02-21T20:59:12Z

docs/controls/MarkdownTextBlock.md

 ### LinkClicked

-Use this event to handle clicking on links for Markdown, by default the MarkdownTextBlock does not handle Clicking on Links.
+Use this event to handle clicking on links or images for Markdown, by default the MarkdownTextBlock does not handle Clicking on Links or Images.


Could you also add a note here that taping any image will raise the LinkClicked event and with the uri of the image?Developers should use the LinkType property to decide how to handle it.

Done. Let me know if there is anything else that i need to update.

Thanks. @WilliamABradley up to you

WilliamABradley · 2018-02-22T06:15:44Z

@avknaidu Can you make the mouse cursor if you hover over the Image into a Hand cursor if the link is clickable? Otherwise it doesn't look clickable, and it follows Hyperlink styling.

WilliamABradley · 2018-02-22T06:49:42Z

As discussed with @nmetulev, we can add Hand cursor clicking in a future PR.

windowstoolkitbot · 2018-02-22T06:50:07Z

This PR is linked to unclosed issues. Please check if one of these issues should be closed: #1091

nmetulev · 2018-02-22T17:39:42Z

@avknaidu, just wanted to give you a quick heads up - after discussing with few folks, there was a conclusion that the current implementation could be a breaking change since developers might already be handling the LinkClicked and they wouldn't expect image click to raise that event. I'm going to submit a quick update to add a new event ImageClicked to handle those cases

avknaidu · 2018-02-22T19:14:34Z

@nmetulev does this mean this PR will not be part of 2.2?

skendrot · 2018-02-22T19:19:04Z

If it's changed to have a new event ImageClicked (for example), similar to LinkClicked then it can go into 2.2

avknaidu · 2018-02-22T19:24:40Z

If you want, I can submit a PR to

add the new event
remove the e.LinkType
update docs.

Let me know.

skendrot · 2018-02-22T19:50:22Z

Please do. Based on what @nmetulev stated above, we would need to back out the change as-is

nmetulev · 2018-02-22T20:48:59Z

That would be great @avknaidu, please do. Do you think you would be able to do it today?

avknaidu · 2018-02-22T20:51:15Z

Sure. Give me 2 hrs. Finishing up some work related stuff. Should i submit another PR?

nmetulev · 2018-02-22T20:55:10Z

Thank you. Yes, please submit a new PR and target the rel/2.2.0 branch

avknaidu closed this Feb 14, 2018

Update changes for ImageTapped

01892b0

avknaidu reopened this Feb 14, 2018

WilliamABradley suggested changes Feb 16, 2018

View reviewed changes

nmetulev reviewed Feb 18, 2018

View reviewed changes

Change return type from String to Enum LinkReturnType

8b6ab76

nmetulev added the controls 🎛️ label Feb 19, 2018

nmetulev approved these changes Feb 19, 2018

View reviewed changes

nmetulev reviewed Feb 19, 2018

View reviewed changes

Updated Headers

15eec4b

Fixing UnhookEvents

beb643c

skendrot suggested changes Feb 19, 2018

View reviewed changes

Simpifying if's

70984c3

skendrot approved these changes Feb 20, 2018

View reviewed changes

Checking Link for Absolute

7ff44e8

Updated docs

f71765f

nmetulev reviewed Feb 21, 2018

View reviewed changes

changing docs to show Note.

aea1d2f

nmetulev approved these changes Feb 21, 2018

View reviewed changes

WilliamABradley approved these changes Feb 22, 2018

View reviewed changes

WilliamABradley merged commit 52f562f into CommunityToolkit:master Feb 22, 2018

WilliamABradley mentioned this pull request Feb 22, 2018

Markdown image hyperlink #1091

Closed

nmetulev mentioned this pull request Feb 22, 2018

Add support for wrapping images in links #1836

Closed

2 tasks

avknaidu deleted the MarkdownImageTapped branch February 22, 2018 10:46

avknaidu mentioned this pull request Apr 19, 2018

Support for wrapping images in links #1984

Merged

4 tasks

		// THE CODE OR THE USE OR OTHER DEALINGS IN THE CODE.
		// ******************************************************************

Uh oh!

Implementing Tapped on Images in MarkdownTextBlock #1807

Implementing Tapped on Images in MarkdownTextBlock #1807

Uh oh!

Conversation

avknaidu commented Feb 13, 2018

PR Type

What is the current behavior?

PR Checklist

What is the new behavior?

Does this PR introduce a breaking change?

Uh oh!

avknaidu commented Feb 13, 2018

Uh oh!

skendrot commented Feb 13, 2018

Uh oh!

avknaidu commented Feb 13, 2018

Uh oh!

skendrot commented Feb 13, 2018

Uh oh!

avknaidu commented Feb 13, 2018

Uh oh!

WilliamABradley commented Feb 14, 2018

Uh oh!

nmetulev commented Feb 14, 2018

Uh oh!

skendrot commented Feb 14, 2018

Uh oh!

nmetulev commented Feb 14, 2018

Uh oh!

avknaidu commented Feb 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmetulev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmetulev Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avknaidu commented Feb 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skendrot left a comment

Choose a reason for hiding this comment

Uh oh!

nmetulev commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nmetulev commented Feb 20, 2018

Uh oh!

avknaidu commented Feb 20, 2018

Uh oh!

WilliamABradley commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avknaidu commented Feb 21, 2018

Uh oh!

nmetulev commented Feb 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmetulev Feb 19, 2018 •

edited

Loading

nmetulev commented Feb 20, 2018 •

edited

Loading

WilliamABradley commented Feb 21, 2018 •

edited

Loading

WilliamABradley commented Feb 22, 2018 •

edited

Loading

skendrot commented Feb 22, 2018 •

edited

Loading