-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Implementing Relative Links/Images/Emails in MarkdownTextBlock #1639
Changes from 9 commits
3201148
7aad125
c9404c1
7e10f81
a98b757
20d6756
0fbfba7
e8a978b
e164840
909cc1e
7948c42
1c6809f
c87de47
457d0fb
d5441a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
using Windows.System; | ||
using Windows.UI.Xaml; | ||
using Windows.UI.Xaml.Controls; | ||
using Windows.UI.Popups; | ||
|
||
namespace Microsoft.Toolkit.Uwp.SampleApp.SamplePages | ||
{ | ||
|
@@ -69,7 +70,14 @@ private void SetInitalText(string text) | |
|
||
private async void MarkdownText_LinkClicked(object sender, UI.Controls.LinkClickedEventArgs e) | ||
{ | ||
await Launcher.LaunchUriAsync(new Uri(e.Link)); | ||
if (e.Link.StartsWith("../") || e.Link.StartsWith("/")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should put the # here as well, maybe we should have a helper method for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where do we put all Helper Methods? I see Also i see Url's being handled this way in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, Common is for Markdown still, and the e-mail regex is in the TextBoxRegEx control as well. @nmetulev would adding an extensions namespace to Microsoft.Toolkit.Uwp make sense here or use the existing extensions namespace under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the helper method here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nmetulev There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it used outside of the controls package? If it's only used here, I'd try to keep it internal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nmetulev Issue here is it cannot be internal because the same Link check is Happening on SampleApp Project. Only way we can do this is to move this to Microsoft.Toolkit.UWP.UI so that it can be used in both places. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, i missed that this is in the sample app. Why not use the standard way of checking for a relative URI?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now for relative URI's we only check if URL's start with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly, I like leaving it up to the developer to decide what to check, so I'm good with the current implementation :) |
||
{ | ||
await new MessageDialog("Masked relative links needs to be manually handled.").ShowAsync(); | ||
} | ||
else | ||
{ | ||
await Launcher.LaunchUriAsync(new Uri(e.Link)); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,6 +184,24 @@ public Thickness CodeBorderThickness | |
typeof(MarkdownTextBlock), | ||
new PropertyMetadata(null, OnPropertyChangedStatic)); | ||
|
||
/// <summary> | ||
/// Gets or sets the Prefix of Image Link. | ||
/// </summary> | ||
public string ImageLinkPrefix | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was thinking since these aren't actually links, should this name be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be a better name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason behind There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @avknaidu you mean so that the developer doesn't have to add the detection and relative link parsing to their In that case, I'd suggest There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @michael-hawker . Done. I will push the commit shortly. |
||
{ | ||
get { return (string)GetValue(ImageLinkPrefixProperty); } | ||
set { SetValue(ImageLinkPrefixProperty, value); } | ||
} | ||
|
||
/// <summary> | ||
/// Gets the dependency property for <see cref="ImageLinkPrefix"/>. | ||
/// </summary> | ||
public static readonly DependencyProperty ImageLinkPrefixProperty = DependencyProperty.Register( | ||
nameof(ImageLinkPrefix), | ||
typeof(string), | ||
typeof(MarkdownTextBlock), | ||
new PropertyMetadata(string.Empty, OnPropertyChangedStatic)); | ||
|
||
/// <summary> | ||
/// Gets or sets the brush used to render the text inside a code block. If this is | ||
/// <c>null</c>, then Foreground is used. | ||
|
@@ -1243,6 +1261,14 @@ private async void Hyperlink_Click(Hyperlink sender, HyperlinkClickEventArgs arg | |
/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns> | ||
async Task<ImageSource> IImageResolver.ResolveImageAsync(string url, string tooltip) | ||
{ | ||
if (!Uri.TryCreate(url, UriKind.Absolute, out Uri uri)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest creating the uri outside of the if block, as if this does succeed then we don't have to recreate the Uri object again to pass to the BitmapImage on line 1281. Then on line 1268, you can create a Uri object there and reuse the uri instance. Then it just gets passed directly into the BitmapImage constructor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should actually be able to just use the uri object outside of this if block, it should still be in scope. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, apparently it does leak into the outer scope, that's a bit odd. Apparently There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it's by design, to be used in scenarios exactly like this |
||
{ | ||
if (!string.IsNullOrEmpty(ImageLinkPrefix)) | ||
{ | ||
url = string.Format("{0}{1}", ImageLinkPrefix, url); | ||
} | ||
} | ||
|
||
var eventArgs = new ImageResolvingEventArgs(url, tooltip); | ||
ImageResolving?.Invoke(this, eventArgs); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
using System; | ||
using System.Collections.Generic; | ||
using Microsoft.Toolkit.Uwp.UI.Controls.Markdown.Helpers; | ||
using System.Text.RegularExpressions; | ||
|
||
namespace Microsoft.Toolkit.Uwp.UI.Controls.Markdown.Parse | ||
{ | ||
|
@@ -193,9 +194,16 @@ internal static Common.InlineParseResult Parse(string markdown, int start, int m | |
} | ||
|
||
// Check the URL is okay. | ||
if (!IsUrlValid(url)) | ||
if (!IsUrlEmail(url)) | ||
{ | ||
return null; | ||
if (!IsUrlValid(url)) | ||
{ | ||
return null; | ||
} | ||
} | ||
else | ||
{ | ||
tooltip = url = string.Format("mailto:{0}",url); | ||
} | ||
|
||
// We found a regular stand-alone link. | ||
|
@@ -264,6 +272,16 @@ public void ResolveReference(MarkdownDocument document) | |
ReferenceId = null; | ||
} | ||
|
||
/// <summary> | ||
/// Checks if the given URL is an Email. | ||
/// </summary> | ||
/// <param name="url"> The URL to check. </param> | ||
/// <returns> <c>true</c> if the URL is valid; <c>false</c> otherwise. </returns> | ||
private static bool IsUrlEmail(string url) | ||
{ | ||
return Regex.IsMatch(url, "(?:[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*|\"(?:[\\x01-\\x08\\x0b\\x0c\\x0e-\\x1f\\x21\\x23-\\x5b\\x5d-\\x7f]|\\\\[\\x01-\\x09\\x0b\\x0c\\x0e-\\x7f])*\")@(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?|\\[(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?|[a-z0-9-]*[a-z0-9]:(?:[\\x01-\\x08\\x0b\\x0c\\x0e-\\x1f\\x21-\\x5a\\x53-\\x7f]|\\\\[\\x01-\\x09\\x0b\\x0c\\x0e-\\x7f])+)\\])"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nmetulev didn't we have a discussion on e-mail regex parsing? I was trying to find the link we found with the reference to this. It'd be nice to put the url reference in a comment here. Also, we use this elsewhere, like the TextBoxRegEx (though that didn't seem to be the one we talked about). So, it might be nice to have a centralized helper for e-mail validation that can be used in all places? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Found it. Yeah, it'd be nice to have a comment with our reference for the future: But since we also use it in TextBoxRegEx, it might be nice to pull it out into its own helper that can be used in both places. Maybe as a string extension? @nmetulev thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, @avknaidu, do you think you can create string extensions and add this method to it to be used with this control and the TextBoxRegEx? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nmetulev Done. Waiting for an answer on the question above to push the commit. |
||
} | ||
|
||
/// <summary> | ||
/// Checks if the given URL is allowed in a markdown link. | ||
/// </summary> | ||
|
@@ -272,7 +290,7 @@ public void ResolveReference(MarkdownDocument document) | |
private static bool IsUrlValid(string url) | ||
{ | ||
// URLs can be relative. | ||
if (url.StartsWith("/") || url.StartsWith("#")) | ||
if (url.StartsWith("/") || url.StartsWith("#") || url.StartsWith("../")) | ||
{ | ||
return true; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michael-hawker I thought that was for control documentation. This is the initial content that loads when MarkdownTextblock loads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry, missed this was in the sample app.