@kuhlenh should we create a fwklink instead here ?

Or an aka.ms link. Correct: no harcoding URLs though.

done

💡 Try to avoid changes to unrelated lines in the file. Either of the following approaches is acceptable:

1. Disable the IDE feature which is causing this whitespace to be removed when the file is edited and/or saved
2. When committing changes to Git, commit the changes on a line-by-line basis while manually avoiding the lines which only have whitespace changes

I don't see any option in Visual Studio to disable the auto removing trailing whitespace, maybe we should build one? ;)
https://social.msdn.microsoft.com/Forums/vstudio/en-US/3c2eb7a1-3ddc-4b67-8105-ee30ab7661d2/how-to-prevent-visual-studio-from-removing-all-trailing-whitespaces?forum=vcgeneral

I figured out if I append ?w=1 at the end of the GitHub url, it does not show the whitespace change. check out this: https://github.com/dotnet/roslyn/pull/24279/files?w=1

💡 The height for this row should (probably) be automatically determined based on the row content.

<RowDefinition Height="Auto" />

done

❔ Is this possible?

could you pls elaborate?

Is it possible for e.Uri to be null? It seems like this is an unnecessary null check.

I searched through the Roslyn solution code base, and it looks like we always check if the uri is null before calling BrowserHelper.StartBrowser(uri). And I prefer to do unnecessary check than the possibility of running into NullReferenceException later..

💡 Add the space in the view, not here.

done

💭 I would have expected the text to be set using the following as a child element, before the hyperlink:

<Run Text="..."/>

❓ Does this need TextWrapping="Wrap" for cases where the dialog is not wide enough?

done

💡 You should be able to use Margin="2,0" on either the Hyperlink or the nested TextBlock element to add the space.

Added after the CodeStylePageHeader text
Hyperlink does not support Margin; if I add Margin to TextBlock, then I can see the hyperlink underline actually starts with an empty space

💡 Place fields above properties

💡 This field can be made readonly

done

❓ In our normal pattern to create wrapper properties for text like this? It seems easier to simply bind the control directly to ServicesVSResources.Learn_more.

Yes, I think there was some screwy problem if you did it directly. @dpoeschl might remember?

".editorconfig files"? (with the dot). People who know what the file is might understand this better then, as right now it's less obvious that's the mechanism being used here.

are you talking about the text? I will wait @kuhlenh to comment on this.

Good call Jason. I think that sounds better.

done

Or an aka.ms link. Correct: no harcoding URLs though.

const so that way it can't be changed.

done

Yes, I think there was some screwy problem if you did it directly. @dpoeschl might remember?

💡 Can you make sure that the added items are all used (remove unused usings)?

done

Is it possible for e.Uri to be null? It seems like this is an unnecessary null check.

❕ Can you get a second reviewer to confirm that this is an acceptable alternative to setting the Margin property on the Learn More text box to provide the spacing?

@jasonmalinowski could you take a look at this? Thanks

Doing a space this way is what I'd prefer actually since I think it's a better semantic representation of "what we want". Imagine you're a screen reader processing this. The space it'll know is a space and is a word break.

@jasonmalinowski Thanks, I was just asking since it differed from another instance of a similar feature in the project. 👍

@sharwell I wouldn't assume in this case that one had a reason for it.

Doing a space this way is what I'd prefer actually since I think it's a better semantic representation of "what we want". Imagine you're a screen reader processing this. The space it'll know is a space and is a word break.

Nit: const variables should be PascalCased.

done