Skip to content
This repository was archived by the owner on Dec 19, 2018. It is now read-only.

Removed newlines at the end of razor comments #482

Merged
merged 1 commit into from
Aug 13, 2015
Merged

Conversation

ajaybhargavb
Copy link
Contributor

Issue - #428

This does not include the fix for the issue mentioned here #428 (comment).
Will add and update tests once the design is finalized.

@NTaylorMullen @dougbu

@@ -134,12 +134,31 @@ protected void SkipToAndParseCode(Func<HtmlSymbol, bool> condition)
{
if (last != null)
{
Accept(last);
if (!startOfLine || last.Type != HtmlSymbolType.WhiteSpace)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for? Looks like it may re-order the symbols improperly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not for this condition, the whitespace preceding the razor comment will show up as part of the next line.

<p>
    @**@
</p>

should produce

<p>
</p>

instead of this

<p>
    </p>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh. Instead of skipping the last we should conditionally accept it, mark its code generator as null (if there wasn't something prior to it like <p>hi</p> @* abcd *@) to not mess up tooling.

@NTaylorMullen
Copy link

@ajaybhargavb ajaybhargavb changed the title [Design] Removed newlines at the end of razor comments Removed newlines at the end of razor comments Aug 12, 2015
@ajaybhargavb
Copy link
Contributor Author

Updated.

@@ -134,12 +134,31 @@ protected void SkipToAndParseCode(Func<HtmlSymbol, bool> condition)
{
if (last != null)
{
if (startOfLine && last.Type == HtmlSymbolType.WhiteSpace)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add a comment explaining this one 😄. This parser loop can be confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance there's more than one symbol waiting to be output? Concern is using the wrong generator for non-whitespace.

@NTaylorMullen
Copy link

Design looks good. Ensure that this works (pretty sure it will, but want to double check):

<p>
  @* Hello *@    @* World *@
</p>

@NTaylorMullen
Copy link

Just to note, you may want to label this PR as part 1/X where X is how many parts you want to take to apply the same niceness to other block level elements in Razor 😄

RazorComment();

// Handle the whitespace and newline at the end of a razor comment.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this only consume the whitespace if startOfLine is true? Shouldn't the final newline be removed in (less common I'm sure but possible) cases like @* hello *@ @* world *@?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked offline; I got it.

@dougbu
Copy link
Contributor

dougbu commented Aug 12, 2015

@ajaybhargavb
Copy link
Contributor Author

Updated.

@dougbu
Copy link
Contributor

dougbu commented Aug 13, 2015

Very close

new MarkupTagBlock(
Factory.Markup("</p>"))
));
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<p>
@* abcd *@

</p>
<p>

@* abcd *@
</p>
<p>
@* abcd *@ Extra @* efgh *@
</p>
<p>
@* 
abcd 
*@
</p>

Intermingle some content in the above Razor comments just for variation instead of always doing @**@.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@NTaylorMullen
Copy link

⌚ for extra test (#482 (comment))

@ajaybhargavb
Copy link
Contributor Author

Updated.

@NTaylorMullen
Copy link

:shipit:

@dougbu
Copy link
Contributor

dougbu commented Aug 13, 2015

:shipit: if we decide to change things related to my new question, can be done later.

@ajaybhargavb ajaybhargavb merged commit 71c4c8d into dev Aug 13, 2015
@ajaybhargavb ajaybhargavb deleted the razor-comment-fix branch August 13, 2015 22:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants