[Experiment] Markdig based MarkdownTextBlock #480

nerocui · 2023-07-23T18:15:38Z

This PR adds a new experiment: MarkdownTextBlock. It's a new take on the existing MarkdownTextBlock in the community toolkit. This implementation use the markdig library for parsing markdown syntax.

This initial implementation contains only early work. After this, the plan is to make it more generic, extensible and customizable.

screenshots

CosmicPredator

Looks good to me...!

itsWindows11

Also I guess might want to use ArrayPool and (ReadOnly)Span if possible here.

components/MarkdownTextBlock/src/TextElements/MyImage.cs

components/MarkdownTextBlock/src/Renderers/ObjectRenderers/HtmlBlockRenderer.cs

michael-hawker

Did a high-level pass, some initial comments. Some things we can do now, some things to think about for later.

components/MarkdownTextBlock/samples/MarkdownTextBlockCustomSample.xaml.cs

components/MarkdownTextBlock/src/TextElements/MyFlowDocument.cs

components/MarkdownTextBlock/src/Renderers/UWPRenderer.cs

components/MarkdownTextBlock/src/MarkdownConfig.cs

components/MarkdownTextBlock/src/MarkdownTextBlock.xaml

components/MarkdownTextBlock/src/MarkdownUIBuilder.cs

michael-hawker · 2023-07-24T22:26:36Z

Ah, also looks like XAML Styler needs to be run, @nerocui you can run the script in the root with -Main and it should clean things up.

niels9001 · 2023-07-25T13:59:32Z

I see that all styling happens in code behind.. would it make sense to move these to separate styles defined in XAML and we load those, when needed, in code behind? Not sure if there would be any performance implications though?

That would allow a developer to easily override a style (like we do here with the current MarkdownTextBlock for the Gallery itself).

components/MarkdownTextBlock/samples/MarkdownTextBlockCustomSample.xaml

michael-hawker · 2023-08-02T20:53:51Z

@nerocui I'll try and take this for a spin later this week. Any discussions you think we need to resolve before an initial merge?

When you have a chance, mind rebasing on the latest main?

You'll want to delete your AdditionalAssemblyInfo.cs file and remove the <Version> tag in your src csproj file.

nerocui · 2023-08-02T21:51:58Z

@michael-hawker I'll address the style in XAML issue later this week, but other than that I don't see anything else for the initial one.

nerocui · 2023-08-07T00:00:50Z

@michael-hawker the latest commit addressed all the comments so far. Please feel free to take another look.

michael-hawker · 2023-08-08T21:56:28Z

components/MarkdownTextBlock/src/MarkdownExtensions.cs

+
+namespace CommunityToolkit.Labs.WinUI.MarkdownTextBlock;
+
+internal class MarkdownExtensions


Why is this empty?

michael-hawker · 2023-08-08T21:59:25Z

components/MarkdownTextBlock/src/MarkdownConfig.cs

+    public string? BaseUrl { get; set; }
+    public IImageProvider? ImageProvider { get; set; }
+    public ISVGRenderer? SVGRenderer { get; set; }
+    public MarkdownThemes Themes { get; set; } = MarkdownThemes.Default;


This should be a property of the control, as otherwise you can't easily set it in XAML here.

Like someone's going to want to do:

<Page.Resources> <controls:MarkdownThemes x:Key="MyMarkdownStyle" Padding="8,4,0,4" H1FontSize="36" ... /> </Page.Resources> <controls:MarkdownTextBlock MarkdownStyle="{StaticResource MyMarkdownStyle}"/>

Need some thought on the name of the class and property here I think. @niels9001 thoughts?

Also, not sure if straight properties will just work on the themes class. And if we want some defaults defined as resource keys in XAML as well... 🤔

michael-hawker · 2023-08-22T16:19:09Z

CI Error:

D:\a\Labs-Windows\Labs-Windows\components\MarkdownTextBlock\src\TextElements\MyList.cs(60,34): 
error CS8524: The switch expression does not handle some values of its input type (it is not exhaustive) involving an unnamed enum value. For example, the pattern '(CommunityToolkit.Labs.WinUI.MarkdownTextBlock.TextElements.BulletType)6' is not covered. [D:\a\Labs-Windows\Labs-Windows\components\MarkdownTextBlock\src\CommunityToolkit.WinUI.Controls.MarkdownTextBlock.csproj]

michael-hawker · 2023-11-20T17:04:49Z

Post-release of 8.0, we've seen a number of folks looking for the MarkdownTextBlock control.

@nerocui I know there's still some questions/discussions around structure/API, but if we could at least get a clean build, we could merge a preliminary version in with tasks in a tracking issue, and make it easier for other folks to start picking it up and providing feedback. Do you still have some time to help at least get it to that point? I don't know what else there may be beyond that warning as error I highlighted above. Thoughts?

nerocui · 2023-11-21T21:29:33Z

@michael-hawker I think that works. I haven't had time to work on this, but I can spend some time to refresh the branch and get to a clean build.

nerocui · 2023-11-22T06:56:27Z

Refreshed and it works locally. It's getting conflict in the submodule. I'm not sure how to resolve that.

niels9001 · 2023-11-22T09:47:36Z

Refreshed and it works locally. It's getting conflict in the submodule. I'm not sure how to resolve that.

Could you try to update from main? Hopefully that would resolve the conflict

nerocui · 2023-11-22T17:42:46Z

@niels9001 I rebased the whole PR from main. Will try again.

niels9001 · 2023-11-22T17:44:17Z

@Arlodotexe can you help out with the conflict?

@nerocui I think the tooling update isn't required for this PR to work, right?

nerocui · 2023-11-22T18:08:32Z

@niels9001 That's right. Tooling is not part of this change.

Arlodotexe · 2024-01-18T17:15:30Z

components/MarkdownTextBlock/src/TextElements/MyCodeBlock.cs

+        var container = new InlineUIContainer();
+        var border = new Border();
+        border.Background = (Brush)Application.Current.Resources["ExpanderHeaderBackground"];
+        border.Padding = _config.Themes.Padding;


We should move assignment of these properties to the template binding.

added label and new icon

nerocui · 2024-01-29T08:57:40Z

@michael-hawker Fixed the test failure and updated the md file to include new icon and "Labs" label. For the reference error, I wasn't sure how to fix it. Do I need to start a separate PR in the tooling repo to add colorcode?

michael-hawker · 2024-01-29T18:19:42Z

@nerocui that's weird, you define the dependency in the dependencies.props so that's all that should be needed... Do you know if you reference the StyleDictionary anywhere?

@Arlodotexe any thoughts?

nerocui · 2024-01-30T11:18:38Z

@michael-hawker I added ColorCode.Core to dependencies.props, so hope that fixes things.

michael-hawker · 2024-01-30T18:19:24Z

@nerocui you already have that defined in your csproj as it's a net standard package, so it shouldn't have to be in the dependencies.props, that file is only for UWP/WinAppSDK/Uno differentiation. So we can put that back. There's something else going on, but not too sure what's going on. @Arlodotexe is going to take a look.

This reverts commit 80c8cf2.

JoeTomkinson · 2024-02-01T13:25:05Z

@michael-hawker I added ColorCode.Core to dependencies.props, so hope that fixes things.

No luck, looks as though it's still failing on depenency-related errors to ColorCode.Core

Arlodotexe · 2024-02-01T23:23:42Z

I believe I've found the source of the issue. The reference to Microsoft.Toolkit.Uwp.UI.Controls.Markdown 7.1.2 on the UWP head contains a transitive reference to ColorCode.Core that's conflicting with the version being referenced in this component.

Removing the reference to the 7.x MarkdownTextBlock control and cleaning all references to it in the head, the build error is resolved. Only reproduces in Release mode, not Debug.

@michael-hawker We'll have to upgrade the heads in tooling to use the Labs version of MarkdownTextBlock over the 7.x version to resolve the build error.

nerocui · 2024-02-02T00:05:00Z

@michael-hawker We'll have to upgrade the heads in tooling to use the Labs version of MarkdownTextBlock over the 7.x version to resolve the build error.

Isn't that kind of chicken and egg problem? We'd need to merge this PR to have tooling to reference to.

michael-hawker · 2024-02-02T04:38:07Z

Ah, thanks for the insight @Arlodotexe. Another case of Toolkit building toolkit strikes again... any suggestions for a path for us to untangle that? Do we have to do an intermittent PR to the tooling submodule to disable the Markdown stuff or should we create a branch for the tooling here to get a clean CI pass all together and then just snap them together?

nerocui · 2024-02-02T05:20:23Z

What if in this PR I don't include ColorCode.Core? I'll just add it back in once this one merges and tooling is able to reference the new control.

Arlodotexe · 2024-02-02T13:38:55Z

@michael-hawker Considering that updates to the tooling will affect both the main repo and Labs, we should probably create a new branch in Tooling for this PR. We can set a ToolkitMarkdownTextBlockSourceProject in Build.Directory.props like we did here, and then set it up here so it can be consumed by the app head.

That will fix our CI issue here, but we won't be able to merge the tooling PR (or this one) into main until we have a NuGet package on the PR feed ready to be used in the CommunityToolkit/Windows repo for the other code path.

When we've got that sorted and can close this PR, we'll update tooling once more to use the package from the public NuGet feed instead of the PR feed.

Arlodotexe · 2024-02-02T14:24:16Z

What if in this PR I don't include ColorCode.Core? I'll just add it back in once this one merges and tooling is able to reference the new control.

Tested locally and can confirm that removing references to ColorCode in this component (both code and packages) also solves the build error.

If we go this route, we can postpone updating the tooling repo to use this new component until we've merged the PR, then we can re-add the ColorCode functionality in a later PR. @michael-hawker This seems more reasonable, what do you think?

michael-hawker · 2024-02-02T18:02:50Z

What if in this PR I don't include ColorCode.Core? I'll just add it back in once this one merges and tooling is able to reference the new control.

@nerocui that's not a bad idea. Would it be hard to isolate? @Arlodotexe sound good?

nerocui · 2024-02-02T18:10:37Z

What if in this PR I don't include ColorCode.Core? I'll just add it back in once this one merges and tooling is able to reference the new control.

@nerocui that's not a bad idea. Would it be hard to isolate? @Arlodotexe sound good?

I'll take a look tonight, but the usage of ColorCode is very limited. So it should be easy.

There will be bug in code block, we will fix it in the next PR.

…dows into neroc/markdigui

nerocui · 2024-02-03T09:46:50Z

Updated with colorcode removed. The code block won't render without it. I don't think we need to work around it for now since we are adding it right back after this right?

niels9001 · 2024-02-03T11:20:29Z

Woohoo! Passed CI

components/MarkdownTextBlock/src/CommunityToolkit.WinUI.Controls.MarkdownTextBlock.csproj

…ls.MarkdownTextBlock.csproj

nerocui · 2024-02-06T00:17:24Z

@michael-hawker shall we merge?

nerocui changed the title ~~Added new experiment markdig based MarkdownTextBlock~~ [Experiment] Markdig based MarkdownTextBlock Jul 23, 2023

CosmicPredator approved these changes Jul 24, 2023

View reviewed changes

itsWindows11 reviewed Jul 24, 2023

View reviewed changes

components/MarkdownTextBlock/src/TextElements/MyImage.cs Show resolved Hide resolved

itsWindows11 reviewed Jul 24, 2023

View reviewed changes

components/MarkdownTextBlock/src/Renderers/ObjectRenderers/HtmlBlockRenderer.cs Outdated Show resolved Hide resolved

michael-hawker reviewed Jul 24, 2023

View reviewed changes

michael-hawker reviewed Jul 27, 2023

View reviewed changes

components/MarkdownTextBlock/samples/MarkdownTextBlockCustomSample.xaml Outdated Show resolved Hide resolved

michael-hawker added the experiment 🧪 Used to track issues that are experiments (or their linked discussions) label Aug 2, 2023

nerocui force-pushed the neroc/markdigui branch from eb611a6 to 0b30f1a Compare August 6, 2023 05:55

michael-hawker reviewed Aug 8, 2023

View reviewed changes

This was referenced Sep 2, 2023

Sample App in Store CommunityToolkit/Windows#195

Closed

Sample App Documentation Rendering Issues CommunityToolkit/Windows#215

Open

MarkdownTextBlock - Python not supported? CommunityToolkit/WindowsCommunityToolkit#4928

Open

michael-hawker mentioned this pull request Sep 19, 2023

Support for RichTextBlock unoplatform/uno#81

Open

michael-hawker mentioned this pull request Nov 20, 2023

Error CS0121 with CommunityToolkit.WinUI.UI.Controls CommunityToolkit/Windows#277

Closed

24 tasks

nerocui force-pushed the neroc/markdigui branch from 843699b to d57c48c Compare November 22, 2023 06:29

nerocui force-pushed the neroc/markdigui branch from 1488f05 to 44779b6 Compare December 14, 2023 07:21

initial commit

07a290a

Arlodotexe reviewed Jan 18, 2024

View reviewed changes

fixed test failures

54ff320

added label and new icon

Merge branch 'main' into neroc/markdigui

2b9ec51

Update Dependencies.props

80c8cf2

nerocui and others added 2 commits January 31, 2024 02:28

Revert "Update Dependencies.props"

cdec30b

This reverts commit 80c8cf2.

Merge branch 'main' into neroc/markdigui

b9afa86

nerocui added 2 commits February 3, 2024 01:42

commented out colorcode for now

98de957

There will be bug in code block, we will fix it in the next PR.

Merge branch 'neroc/markdigui' of https://github.com/nerocui/Labs-Win…

d8c9025

…dows into neroc/markdigui

niels9001 requested review from michael-hawker and Arlodotexe February 3, 2024 11:20

Arlodotexe reviewed Feb 5, 2024

View reviewed changes

components/MarkdownTextBlock/src/CommunityToolkit.WinUI.Controls.MarkdownTextBlock.csproj Show resolved Hide resolved

Update components/MarkdownTextBlock/src/CommunityToolkit.WinUI.Contro…

fa0c518

…ls.MarkdownTextBlock.csproj

Arlodotexe approved these changes Feb 5, 2024

View reviewed changes

michael-hawker merged commit d50095f into CommunityToolkit:main Feb 6, 2024
7 checks passed


		namespace CommunityToolkit.Labs.WinUI.MarkdownTextBlock;

		internal class MarkdownExtensions

[Experiment] Markdig based MarkdownTextBlock #480

[Experiment] Markdig based MarkdownTextBlock #480

Conversation

nerocui commented Jul 23, 2023 • edited

CosmicPredator left a comment • edited

Choose a reason for hiding this comment

itsWindows11 left a comment

Choose a reason for hiding this comment

michael-hawker left a comment

Choose a reason for hiding this comment

michael-hawker commented Jul 24, 2023

niels9001 commented Jul 25, 2023

michael-hawker commented Aug 2, 2023

nerocui commented Aug 2, 2023

nerocui commented Aug 7, 2023

michael-hawker Aug 8, 2023

Choose a reason for hiding this comment

michael-hawker Aug 8, 2023

Choose a reason for hiding this comment

michael-hawker commented Aug 22, 2023

michael-hawker commented Nov 20, 2023

nerocui commented Nov 21, 2023

nerocui commented Nov 22, 2023

niels9001 commented Nov 22, 2023

nerocui commented Nov 22, 2023

niels9001 commented Nov 22, 2023

nerocui commented Nov 22, 2023

Arlodotexe Jan 18, 2024

Choose a reason for hiding this comment

nerocui commented Jan 29, 2024

michael-hawker commented Jan 29, 2024

nerocui commented Jan 30, 2024

michael-hawker commented Jan 30, 2024

JoeTomkinson commented Feb 1, 2024

Arlodotexe commented Feb 1, 2024

nerocui commented Feb 2, 2024

michael-hawker commented Feb 2, 2024

nerocui commented Feb 2, 2024

Arlodotexe commented Feb 2, 2024 • edited

Arlodotexe commented Feb 2, 2024 • edited

michael-hawker commented Feb 2, 2024

nerocui commented Feb 2, 2024

nerocui commented Feb 3, 2024

niels9001 commented Feb 3, 2024

nerocui commented Feb 6, 2024

nerocui commented Jul 23, 2023 •

edited

CosmicPredator left a comment •

edited

Arlodotexe commented Feb 2, 2024 •

edited

Arlodotexe commented Feb 2, 2024 •

edited