Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

loose lists #1304

Merged
merged 3 commits into from Jul 14, 2018
Merged

loose lists #1304

merged 3 commits into from Jul 14, 2018

Conversation

UziTech
Copy link
Member

@UziTech UziTech commented Jul 3, 2018

Marked version: 0.4.0

Markdown flavor: CommonMark

Description

Makes "loose" lists conform to the standard, requiring all items in a loose list to be loose.

This conflicts with #1 in which @chjj takes his own approach to loose lists, which I don't think comes from any standard. My proposed solution to the failing non-standard tests would be to just remove the tests.

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
  • no tests required for this PR.
  • If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR

@UziTech UziTech changed the title [wip] loose lists loose lists Jul 9, 2018
@@ -365,6 +372,10 @@ Lexer.prototype.token = function(src, top) {
if (!loose) loose = next;
}

if (loose) {
listStart.loose = true;
Copy link
Member

Choose a reason for hiding this comment

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

Can this if statement be removed in favor of listStart.loose = loose

Copy link
Member Author

Choose a reason for hiding this comment

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

no because if loose is false and listStart.loose is true I don't want it to change listStart.loose to false.

listStart.loose is true if any of listStart items are loose

@UziTech
Copy link
Member Author

UziTech commented Jul 10, 2018

Since I am removing the token loose_item_start in favor of a .loose property on a list_item_start token this will probably be a breaking change

@@ -1217,28 +1238,20 @@ Parser.prototype.tok = function() {
}
case 'list_item_start': {
body = '';
var loose = this.token.loose;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why you create a new var here instead of using this.token.loose below?

Copy link
Member Author

Choose a reason for hiding this comment

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

because this.token is changed when this.next() is called in the while loop below.

body += !this.token.loose && this.token.type === 'text' would be checking if the text token is loose not the list start token

@styfle
Copy link
Member

styfle commented Jul 14, 2018

@joshbruce @davisjam Can you review this PR?

@Martii Can you check to see if this solves your issue?

Updated, I confirmed it solves that use case ^

@joshbruce joshbruce merged commit 706e07d into markedjs:master Jul 14, 2018
@Martii
Copy link
Contributor

Martii commented Jul 15, 2018

@styfle

Maybe in a little bit I'll have some time... but the Marked demo seems to not be giving me a preview in a clean browser profile. The HTML source that is selectable looks different than I recall.

@Martii
Copy link
Contributor

Martii commented Jul 15, 2018

And now it appeared for preview in a new tab... love quirks like this. n/m

@styfle
Copy link
Member

styfle commented Jul 15, 2018

@Martii I had a chrome extension that was causing an issue with the demo. Disabling the extension fixed it.

That being said, this PR is not released yet and therefore not available in the demo right now.

@Martii
Copy link
Contributor

Martii commented Jul 15, 2018

That being said, this PR is not released yet and therefore not available in the demo right now.

I know... that's why I made the suggestion of having a current HEAD option. Was doing a bazillion things yesterday and never got around to testing the current HEAD myself.

chrome extension that was causing an issue with the demo

...

preview in a clean browser profile.

;) So it was something else... perhaps the CDN wasn't delivering? Anyhow... probably something for a different issue if it continues to crop up. :) Thanks.

This was referenced Apr 7, 2020
zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Marked adds extra/unnecessary <p> tags to list elements
4 participants