Skip to content

Conversation

@isoos
Copy link
Collaborator

@isoos isoos commented Feb 14, 2020

I've started to implement this with CSS-only, but it only worked for a limited case. Rewriting the markdown tree is a bit more work, but creates a consistent experience.

Desktop view:

Screen Shot 2020-02-14 at 14 32 41

@isoos isoos requested a review from jonasfj February 14, 2020 13:35
Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

I like that we try to parse the changelog.

I worry that we are too lenient.

Perhaps we should document a much more strict format, and give a pana penality to packages that fail to parse.

We can land something like this to get on with the new design.

for (final node in nodes) {
final version = (node is m.Element &&
node.children.isNotEmpty &&
node.children.first is m.Text)
Copy link
Contributor

Choose a reason for hiding this comment

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

This implies that styled text is not accepted (that would give a nested m.Element)


String markdownToHtml(String text, String baseUrl, {String baseDir}) {
/// Renders markdown [text] to HTML.
String markdownToHtml(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should split this in three parts along the lines of:

  • m.Document parseMarkdown(text, baseUrl, baseDit)
  • m.Document formatChangelog(m.Document)
  • String markdownToHtml(m.Document)

return null;
}

/// Group corresponding changelog nodes together, if it matches the following
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to document our supported changelog formats publically.

That should of course be handled in another pr.

/// pattern:
/// - version identifiers are the only content in a single line
/// - heading level or other style doesn't matter
/// - optional `v` prefix is accepted
Copy link
Contributor

Choose a reason for hiding this comment

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

@jonasfj suggested other variations that we might want to recognize:

## version 1.0.0
## v. 1.0.0
## V1.0.0

var nodes = document.parseLines(lines);

if (isChangelog) {
nodes = _groupChangelogNodes(nodes).toList();
Copy link
Contributor

Choose a reason for hiding this comment

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

We could consider trying to remove a heading # Changelog to make changelogs appear more uniform.

/// Group corresponding changelog nodes together, if it matches the following
/// pattern:
/// - version identifiers are the only content in a single line
/// - heading level or other style doesn't matter
Copy link
Contributor

Choose a reason for hiding this comment

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

@jonasfj suggests we might want to limit ourselves to allowing h1 and h2

@isoos isoos mentioned this pull request Feb 17, 2020
8 tasks
@isoos
Copy link
Collaborator Author

isoos commented Feb 17, 2020

I agree with all of the suggestions, but the feature space exploded a bit, and it is slightly unrelated to the UI changes. I'd rather land a PR with the UI structure, and iterate on the other features separately. I've collected the requests that were not addressed yet in the PR here: #3331, let me know which of those do you want to land with this PR.

Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

LGTM

@isoos isoos merged commit e723d6a into dart-lang:master Feb 17, 2020
@isoos isoos deleted the changelog branch February 17, 2020 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants