Bringing UniversalMarkdown to the party #772

QuinnDamerell · 2017-01-09T05:24:39Z

Starting the process of bringing UniversalMarkdown into the toolkit as MarkdownTextBlock. I believe everything is in order except for the style issues in the unit tests and adding a documentation page for the control.

#268

…. Added the control to the sample, added the unit test to the test folder, started some cleanup work of the code style.

… of the style errors in the main code.

dnfclas · 2017-01-09T05:24:44Z

Hi @QuinnDamerell, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

dnfclas · 2017-01-09T05:32:24Z

@QuinnDamerell, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

hermitdave · 2017-01-09T10:21:31Z

@QuinnDamerell you seem to be missing a csproj in commit

QuinnDamerell · 2017-01-09T17:36:32Z

@hermitdave any idea which? The only new project file is the unit test project. Is that not showing up?

hermitdave · 2017-01-09T18:05:15Z

MSBuild auto-detection: using msbuild version '14.0' from 'C:\Program Files (x86)\MSBuild\14.0\bin'.

C:\Users\appveyor\AppData\Local\Temp\1\NuGet-Scratch\0uxgbmhn.nn5.targets(37,5): error MSB3202: The project file "..\UniversalMarkdown\UniversalMarkdown.csproj" was not found. [C:\projects\uwpcommunitytoolkit\UnitTests\UnitTests.MarkdownTextBlock\MarkdownTextBlockUnitTests.csproj]

QuinnDamerell · 2017-01-10T15:59:12Z

Good catch, idk why my computer didn't blow up about that. It is fixed now.

# Conflicts: # Microsoft.Toolkit.Uwp.UI.Controls/Microsoft.Toolkit.Uwp.UI.Controls.csproj

deltakosh · 2017-01-10T22:45:04Z

I have an issue compiling the project:

deltakosh · 2017-01-10T22:47:42Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/MarkdownTextBlock/MarkdownTextBlockCode.bind

@@ -0,0 +1,44 @@
+using System;


This file is not required as the control is pretty obvious :)

deltakosh · 2017-01-10T22:49:09Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/MarkdownTextBlock/MarkdownTextBlockPage.xaml

+        </Grid.RowDefinitions>
+
+        <TextBlock Grid.Row="0" Text="Regular Text" Margin="12,12,12,0" FontSize="18"/>        
+        <TextBox x:Name="ui_unformattedText" Grid.Row="1" Margin="12" AcceptsReturn="True" TextWrapping="Wrap" />


We should add a scrollviewer and bind its content to the makrdowntextblock to allow users to test it live :)

I actually tried that, but there was an issue where when I got the text back out of the TextBox it was missing some of the new lines required to keep the formatting of the markdown correct. I looked for a little while but couldn't figure it out, which is really confusing because this is exactly how I do it in Baconit, and how the old markdown sample app does it.

I'll have a look this afternoon :)

deltakosh · 2017-01-10T22:49:37Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/MosaicControl/MosaicControlPage.xaml.cs

@@ -22,7 +22,7 @@ public MosaicControlPage()
            InitializeComponent();
        }

-        protected override async void OnNavigatedTo(NavigationEventArgs e)


Good catch!

deltakosh · 2017-01-10T22:51:02Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/Display/MarkdownTable.cs

+    /// </summary>
+    internal class MarkdownTable : Panel
+    {
+        private int columnCount;


private members must starts with _ (sorry)

deltakosh · 2017-01-10T22:55:51Z

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/Helpers/SmartWeakEvent.cs

@@ -0,0 +1,174 @@
+// ******************************************************************


Do you think you can leverage the WeakEventListener: https://github.com/Microsoft/UWPCommunityToolkit/blob/dev/Microsoft.Toolkit.Uwp/Helpers/WeakEventListener.cs ?

Correct me if I'm wrong, but the WeakEventListener looks like something the consumer would have to do. The smart weak events allow the consumer to register just like normal events, but not have to worry about removing the registration when they clean up.

I could remove the SmartWeakEvent logic and just make them normal events, but then the consumer would have to use the WeakEventListener to get the same effect.

You're right...But then the WeakEvent should be moved to Helpers as it could be useful even outside the markdowntextblock:)

I'm fine with it either way. I could move the smart weak events or remove them all together. Whatever you would like, whatever you think feels best for the project.

I would say: go with regular events. We need to stick with what is already in the toolkit and in the SDK.

If people want weak event they can use the WeakEventListener :)

deltakosh · 2017-01-10T22:57:09Z

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

+        /// <summary>
+        /// The default color used for backgrounds of sub elements.
+        /// </summary>
+        private static Color _defaultSubElementBgColor = Color.FromArgb(255, 224, 224, 224);


I think these two properties could be in the template (as a setter)

How does that work exactly? I make them setters in the .xaml and then how do I access them in the cs?

Because you use them as default value for CodeBackGround for instance I would just set CodeBackground to this value in XAML and that's it :)

deltakosh · 2017-01-10T22:59:15Z

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

+            set { SetValue(TextProperty, value); }
+        }
+
+        public static readonly DependencyProperty TextProperty = DependencyProperty.Register(


W try to have all DP at the beginning of the file

(check to make sure I did it right.)

deltakosh · 2017-01-10T23:01:51Z

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

+
+namespace Microsoft.Toolkit.Uwp.UI.Controls
+{
+    public class OnMarkdownReadyArgs : EventArgs


Missing xmldoc
For some reasons the analyzer is not flagging it. Pinging @onovotny and @pedrolamas for help

deltakosh · 2017-01-10T23:02:13Z

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

+
+namespace Microsoft.Toolkit.Uwp.UI.Controls
+{
+    public class OnMarkdownLinkTappedArgs : EventArgs


# Conflicts: # Microsoft.Toolkit.Uwp.SampleApp/Microsoft.Toolkit.Uwp.SampleApp.csproj

paulbartrum · 2017-01-23T22:57:39Z

By the way, there were two compile errors when I tried to build this branch. I had to prepend this. to FindDescendant() and FindVisualAscendant() to fix it. dev doesn't seem to have this problem so I'm guessing this was the result of a bad merge.

…y the parser.

# Conflicts: # Microsoft.Toolkit.Uwp.SampleApp/Microsoft.Toolkit.Uwp.SampleApp.csproj

paulbartrum · 2017-01-24T02:30:12Z

// This test is ignored because it test the true markdown standard, while currently this parser
// only supports the "reddit" flavor of markdown currently. (this difference is quite small)

@QuinnDamerell: That's not quite right. There are some known cases where we parse differently from Reddit, and that's what the tests are checking. They're all pretty small, nit-picky type stuff (but not necessarily easy to fix).

QuinnDamerell · 2017-01-24T03:46:16Z

@paulbartrum ah, so it's the other way around? I thought from looking at them it looked like that.

QuinnDamerell · 2017-01-24T05:16:03Z

I think I have all of the unit test issues resolved now (@pualbartum let me know if I'm wrong).

The last thing I have to resolve is the styling issues, which I hope to get more clarity on. :)

deltakosh · 2017-01-24T18:40:51Z

Excellent! Did you see my responses regarding Style?

deltakosh · 2017-01-24T18:51:36Z

Let me push an update for you :)

QuinnDamerell · 2017-01-24T19:22:24Z

DO IT. JUST DO IT. 😁

Yeah I saw the comment but I still don't quite get it. Feel free to fix it yourself or let me know how to fix it.

deltakosh · 2017-01-24T19:35:24Z

I did it :) but now I need you to allow me to push on your repo :)

QuinnDamerell · 2017-01-25T03:34:28Z

Done and Thanks!

Fixes for PR

deltakosh · 2017-01-25T16:51:40Z

Done
Please give me your thoughts

# Conflicts: # Microsoft.Toolkit.Uwp.SampleApp/Microsoft.Toolkit.Uwp.SampleApp.csproj # Microsoft.Toolkit.Uwp.UI.Controls/Microsoft.Toolkit.Uwp.UI.Controls.csproj

QuinnDamerell · 2017-01-26T04:30:03Z

@deltakosh looks great! thanks for doing that.

Anything else we need to do?

deltakosh · 2017-01-26T04:36:09Z

Nope we are good to go:) I'll merge tomorrow

Fantastic job. Thank you so much!

QuinnDamerell · 2017-01-26T04:37:33Z

Woot! We did it! Thanks for all of your help!

deltakosh · 2017-01-26T16:17:42Z

Please send me your address for a tshirt :) ([email protected])

xied75 · 2017-01-26T16:58:13Z

"your" including?

QuinnDamerell · 2017-01-26T16:59:08Z

Omg there's a shirt!?!?

Thanks again to everyone who helped out with this. 😀

deltakosh · 2017-01-26T17:00:02Z

@xied75 both of you OF COURSE :)

QuinnDamerell added 3 commits January 8, 2017 21:21

Adding the new MarkdownTextBlock control taken from UniversalMarkdown…

982b3af

…. Added the control to the sample, added the unit test to the test folder, started some cleanup work of the code style.

Fixing up the public interface of the Markdown control and fixing all…

d00af79

… of the style errors in the main code.

Adding the MarkdownTextBlock icon.

64b22d8

dnfclas added the cla-required label Jan 9, 2017

QuinnDamerell mentioned this pull request Jan 9, 2017

New control to display MD (Markdown) files #300

Closed

dnfclas added cla-signed and removed cla-required labels Jan 9, 2017

Fixing build error where and old project was still in the project files.

e556e4e

QuinnDamerell added 2 commits January 10, 2017 08:13

Merge branch 'dev' into markdowntextblock

6e39206

# Conflicts: # Microsoft.Toolkit.Uwp.UI.Controls/Microsoft.Toolkit.Uwp.UI.Controls.csproj

Updating headers for the new files.

d2c2396

deltakosh added need documentation 📃 reviewers wanted labels Jan 10, 2017

deltakosh reviewed Jan 10, 2017

View reviewed changes

QuinnDamerell added 2 commits January 11, 2017 09:00

Code review feedback.

fb5fba7

Merge remote-tracking branch 'real/dev' into markdowntextblock

8ac39aa

# Conflicts: # Microsoft.Toolkit.Uwp.SampleApp/Microsoft.Toolkit.Uwp.SampleApp.csproj

QuinnDamerell added 3 commits January 23, 2017 18:24

Adding the ignore flag to the tests that currently aren't supported b…

2057d1c

…y the parser.

Merge remote-tracking branch 'real/dev' into markdowntextblock

d60de6c

# Conflicts: # Microsoft.Toolkit.Uwp.SampleApp/Microsoft.Toolkit.Uwp.SampleApp.csproj

Fixing some merge errors.

fc4233c

Updating the text surrounding the ignored tests.

efb3721

QuinnDamerell and others added 5 commits January 24, 2017 20:45

Merge remote-tracking branch 'real/dev' into markdowntextblock

73c890e

Moving the hyper link events over to the weak event system.

fa5d80c

Cleaning up the markdown unit test project.

6c3c4e4

Fixes for PR

0aea9d4

Merge pull request #1 from deltakosh/markdowntextblock

20ca759

Fixes for PR

Merge remote-tracking branch 'real/dev' into markdowntextblock

e5c9418

# Conflicts: # Microsoft.Toolkit.Uwp.SampleApp/Microsoft.Toolkit.Uwp.SampleApp.csproj # Microsoft.Toolkit.Uwp.UI.Controls/Microsoft.Toolkit.Uwp.UI.Controls.csproj

deltakosh merged commit 896b20a into CommunityToolkit:dev Jan 26, 2017

deltakosh deleted the markdowntextblock branch January 26, 2017 17:00

michael-hawker mentioned this pull request Sep 9, 2020

Split Markdown.* from Controls package. #3437

Merged

7 tasks

		@@ -0,0 +1,174 @@
		// ******************************************************************

Bringing UniversalMarkdown to the party #772

Bringing UniversalMarkdown to the party #772

Uh oh!

Conversation

QuinnDamerell commented Jan 9, 2017 • edited by dotMorten Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Jan 9, 2017

Uh oh!

dnfclas commented Jan 9, 2017

Uh oh!

hermitdave commented Jan 9, 2017

Uh oh!

QuinnDamerell commented Jan 9, 2017

Uh oh!

hermitdave commented Jan 9, 2017

Uh oh!

QuinnDamerell commented Jan 10, 2017

Uh oh!

deltakosh commented Jan 10, 2017

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

paulbartrum commented Jan 23, 2017

Uh oh!

paulbartrum commented Jan 24, 2017

Uh oh!

QuinnDamerell commented Jan 24, 2017

Uh oh!

QuinnDamerell commented Jan 24, 2017

Uh oh!

deltakosh commented Jan 24, 2017

Uh oh!

deltakosh commented Jan 24, 2017

Uh oh!

QuinnDamerell commented Jan 24, 2017

Uh oh!

deltakosh commented Jan 24, 2017

Uh oh!

QuinnDamerell commented Jan 25, 2017

QuinnDamerell commented Jan 9, 2017 •

edited by dotMorten

Loading

deltakosh commented Jan 26, 2017 •

edited

Loading