[SuperEditorMarkdown] Add header alignments

[lamnhan066]

@matthew-carroll Here are my implementations for the header alignments.

Fixes #865

[matthew-carroll]

@vnniz as I mentioned in the ticket, I'm still trying to determine what syntax you're proposing. Can you please return to the ticket and explain the Markdown syntax? We need to decide that this is something we want before taking time to review an implementation.

[angelosilvestre]

I discussed with Matt about the syntax and we agreed we should add the alignment notation in the same line of the header, after the #.

If you want to proceed with this PR, please update it to the following syntax:

#:--- Header or # Header => Left Alignment
#:---: Header => Center Alignment
#---: Header => Right Alignment
#-::- Header => Justify Alignment

Also, please add tests for parsing a header with alignment, also updating the 'example doc' and 'example doc 1' tests to include headers with all alignments.

If you are not willing to make those changes, please let me know and I will create a new PR based on this one.

[lamnhan066]

@angelosilvestre As we discussed at #865, I'll keep the current format of the header alignment syntax. I have resolved some issues that you mentioned and added some tests. Can you give it a look and do you think the added tests are enough?

[angelosilvestre]

I left some comments. After addressing them you can tag @matthew-carroll for his review.

[lamnhan066]

@matthew-carroll I have formatted the code and improved the comments. Please give it a look.

[matthew-carroll]

@vnniz there seems to be a number of merge conflicts with your PR. Please rebase against main and resolve those.

[lamnhan066]

@matthew-carroll done.

[matthew-carroll]

This comment is in a strange location. It's unclear how it relates to the code. After reading this comment, I go down a few lines, and we're adding the alignment token, so it's confusing to see a comment that says "there is no need to add the alignment token". Please ensure that comments appear in places that clarify code behavior, rather than confuse it.

[lamnhan066]

In the next line, we have textAlign != 'left' condition, which means we don't need to add the left alignment token to the markdown when we serialize a header node (the same as ParagraphNodeSerializer). Do you have any better comment for it?

[matthew-carroll]

The primary issue is the location of the comment. The code directly below that comment doesn't implement "no need to add the alignment token". This comment actually refers to a non-existent else statement.

The code that actually exists is code that adds an alignment token. Not code that avoids adding an alignment token.

[lamnhan066]

// Left alignment is the default, so there is no need to add the alignment token.
if (markdownSyntax == MarkdownSyntax.superEditor && textAlign != null && textAlign != 'left') {
   final alignmentToken = _convertAlignmentToMarkdown(textAlign);
   if (alignmentToken != null) {
      buffer.writeln(alignmentToken);
  }
}

I think it would be clear if the comment is "Left alignment is the default, so there is no need to add the left alignment token.".

[matthew-carroll]

You're still phrasing the comment in terms of what the code ISN'T doing, instead of what the code IS doing. Please write code comments in terms of what the code IS doing.

[lamnhan066]

This comment has been updated as you requested.

[lamnhan066]

@matthew-carroll I have updated some comments and reviews.

[lamnhan066]

In the next line, we have textAlign != 'left' condition, which means we don't need to add the left alignment token to the markdown when we serialize a header node (the same as ParagraphNodeSerializer). Do you have any better comment for it?

[matthew-carroll]

LGTM - Thanks for the contribution.

[matthew-carroll]

@angelosilvestre can you please cherry pick this to stable?