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

Fixes issue #11 (nested ems) #12

Merged
merged 3 commits into from
Feb 4, 2020
Merged

Conversation

matthew0x40
Copy link
Contributor

@matthew0x40 matthew0x40 commented Feb 2, 2020

Although a nested <em> tag shouldn't affect how the rendered markdown looks without any special CSS styling, I think this library should still reflect Discord's parsing behavior as closely as possible.


This PR modifies the default rules for em:

  • adds state flag inEmphasis when parsing contents (flag would still be false if parsing outermost em)
  • if inEmphasis is true, then the parse function returns the parsed node's contents rather than the node itself to exclude the <em> tags

Added new tests to oddities.test.js to test this behavior.

Failing tests: Overall tests may fail due to bugs in the inlineCode rule, my other PR should fix those bugs.

@Torvin
Copy link

Torvin commented Feb 3, 2020

Wouldn't it be cleaner to just output a plain-text node from parse instead of fixing it in html?

@matthew0x40
Copy link
Contributor Author

I did try that, but <em> can contain other elements such as <u> so I can't return a plain text type from parse.

Here's one of the tests I added:

expect(markdown.toHTML('_hello world *foo __blah__ bar* hello world_'))
	.toBe('<em>hello world foo <u>blah</u> bar hello world</em>');

Trying to return a plain-text node resulted in <em>hello world [object Object] hello world</em>

@Torvin
Copy link

Torvin commented Feb 3, 2020

Hmmm... I tried this and it worked:

em: Object.assign({ }, markdown.defaultRules.em, {
	parse: function(capture, parse, state) {
		const parsed = markdown.defaultRules.em.parse(capture, parse, Object.assign({ }, state, { inEmphasis: true }));
		return state.inEmphasis ? parsed.content : parsed
	}
}),

@matthew0x40
Copy link
Contributor Author

Oh yes, that does work! I must've messed something up when I was trying to do it, gave up too quick. Thanks for catching that!

@brussell98 brussell98 merged commit f4ff9ae into brussell98:master Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants