Support for wrapping images in links #1984
Conversation
|
I do not think Sample needs an update. But if you want me to update sample, let me know. |
There was a problem hiding this comment.
My expectation here clicking the image would invoke the LinkClicked event and not the ImageClicked event. For example [](http://linkurl) would render the image and when clicked, it would invoke LinkClicked passing the http://linkurl. Instead, the ImageClicked is invoked passing the http://imageurl.
|
@avknaidu Include an example in the default .md file in the sample app. |
|
I made changes to open assigned Link instead of opening Image.
Even though this is a link, Since you are clicking on the image, It will still invoke
Done. |
| } | ||
|
|
||
| // Check the scheme is allowed. | ||
| bool schemeIsAllowed = false; |
There was a problem hiding this comment.
could probably do without this flag .. just return true immediately in the loop .. and return false after the loop
|
|
||
| // Check the scheme is allowed. | ||
| bool schemeIsAllowed = false; | ||
| foreach (var scheme in HyperlinkInline.KnownSchemes) |
There was a problem hiding this comment.
you can get the scheme directly from the result uri above and just check if the KnownSchemes contains it.
| /// If this is a reference-style link, attempts to converts it to a regular link. | ||
| /// </summary> | ||
| /// <param name="document"> The document containing the list of references. </param> | ||
| public void ResolveReference(MarkdownDocument document) |
There was a problem hiding this comment.
does this need to be public?
There was a problem hiding this comment.
Yes. Because this method is being called from a Protected Method and I get an accessibility error.
| @@ -378,18 +378,45 @@ renders in | |||
|
|
|||
| You can also specify image width like this: | |||
|
|
|||
| >\!\[SVG logo](https://upload.wikimedia.org/wikipedia/commons/0/02/SVG_logo.svg =32) (width is set to 32) | |||
| >\!\[SVG logo](https\://upload.wikimedia.org/wikipedia/commons/0/02/SVG_logo.svg =32) (width is set to 32) | |||
There was a problem hiding this comment.
why does the : need escaping now and not before - is it a breaking change?
There was a problem hiding this comment.
This is just a UI Choice. Before It used to render in example. Just for consistency, i added the escape character. or else even that URL will be rendered.
|
On the ImageClicked vs LinkClicked, I think there is still benefit in having both to handle different cases. However, I would expect the LinkClicked to be fired instead of the ImageClicked event when an image is wrapped with a link. |
|
@nmetulev I will make remaining change (LinkClicked when tapped on Image) tomorrow. In the mean time, if you find any other issues, let me know. @WilliamABradley could you please review the PR? |
@nmetulev When Images are wrapped as Links, it will now fire |
| /// If this is a reference-style link, attempts to converts it to a regular link. | ||
| /// </summary> | ||
| /// <param name="document"> The document containing the list of references. </param> | ||
| public void ResolveReference(MarkdownDocument document) |
|
4 of the 6 tests
are failing because I added 2 new properties 2 of the 6 tests
are failing because we are taking the scheme from URI instead of the list. This changes the tests that are looking for Let me know if you want me commit with changes. Below is the output after changes.
|
|
ping @nmetulev |
|
Yes, please make sure the tests are updated to make sure we have the appropriate coverage |
|
@nmetulev ping |
|
@WilliamABradley ping |
|
@WilliamABradley do you think you verify this PR today? I have PR's for #2069 , #1849 and #1383 lined up but cannot create a PR because of the commits on this PR. I merged changes into Master by mistake. |
|
I left a comment on the issue itself that we can wrap the image in hyperlink button. That way we can get the hand cursor. I already made that change in another PR. Will push for review. |
Issue: #1836
PR Type
What kind of change does this PR introduce?
Feature
What is the current behavior?
When using Wrapped ImageLinks like Below it will not render.
[](https://github.com/nmetulev)What is the new behavior?
Will render in Markdown TextBlock
PR Checklist
Please check if your PR fulfills the following requirements:
[x] Tested code with current supported SDKs
[x] Docs have been added/updated which fit documentation template. (for bug fixes / features)
[x] 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)
[ ] Header has been added to all new source files (run build/UpdateHeaders.bat)
[x] Contains NO breaking changes
Tasks