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

Add support for phrasing content #58

Merged
merged 7 commits into from
May 13, 2019
Merged

Add support for phrasing content #58

merged 7 commits into from
May 13, 2019

Conversation

wooorm
Copy link
Member

@wooorm wooorm commented Apr 25, 2019

Closes GH-56.
Closes GH-57.
Closes GH-58.

@wooorm wooorm changed the title add test Add support for phrasing content Apr 25, 2019
@wooorm wooorm marked this pull request as ready for review April 25, 2019 12:04
@wooorm
Copy link
Member Author

wooorm commented Apr 25, 2019

@d4rekanguok here’s how I’d do it. Could you check? The slugs/URLs aren’t perfect, though. And they’re different from GitHub. Although this has always been a problem, so it isn’t new.

@d4rekanguok
Copy link

I tested it out & it worked great!! Can't wait to see this get merged. Thank you again for the swift response.

var index = -1

while (++index < length) {
result = result.concat(one(children[index]))
Copy link
Member

Choose a reason for hiding this comment

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

One option could be to only copy text nodes

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... that gets tricky for image nodes and the like..

Copy link
Member

@BarryThePenguin BarryThePenguin left a comment

Choose a reason for hiding this comment

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

Without doing too much to turn other nodes into text nodes, this is probably the simplest approach

Pretty sure this won't introduce any breaking changes either?

Otherwise, this looks good 👍

@wooorm
Copy link
Member Author

wooorm commented Apr 28, 2019

Thanks for reviewing both!

@BarryThePenguin This will definitely break things! Potentially for the better.
I think people don’t typically include syntax in headings, except for a big part of the audience where they use inline code to document API functions, which we’re doing in this project as well.
I think copying that syntax over is actually better. But that might be an opinion, so we could also make it an option?

@wooorm
Copy link
Member Author

wooorm commented May 13, 2019

@BarryThePenguin What do you think re my previous comment?

Copy link
Member

@BarryThePenguin BarryThePenguin left a comment

Choose a reason for hiding this comment

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

Yup. That makes sense. The expected test output has changed, so making it a breaking change makes sense to me 👍🏻

One thought I had.. If this new behaviour were optional, we could release it under an option, which defaults to disabled. If we wanted this to then be the default behaviour, we could release a major version that reflects this.

I'm happy to take care of that if that's how we want to move forward?

@wooorm
Copy link
Member Author

wooorm commented May 13, 2019

@BarryThePenguin Regarding keeping the current behaviour as the default option: I think the new behaviour makes more sense as the default. I propose releasing this as a major, and if issues come in to investigate what users would like: 1) the previous behaviour, or 2) some sort of node filtering where they can choose which nodes to keep, etc, or 3) maybe something else.

For this PR: I just noticed that images are also included in the TOC. Is that a good idea? I don’t think people really use images in headings (except maybe the main heading of a readme, but that doesn’t appear in the TOC), so we can either a) not copy images, or b) wait for what people want and potentially allow filtering (2 above) of nodes later.

@BarryThePenguin
Copy link
Member

Sounds good 👍🏻

@wooorm wooorm merged commit 6fbb123 into master May 13, 2019
@wooorm wooorm deleted the phrasing-content branch May 13, 2019 14:58
@wooorm wooorm added ⛵️ status/released 🗄 area/interface This affects the public interface 🦋 type/enhancement This is great to have 🧑 semver/major This is a change labels Aug 12, 2019
@wooorm wooorm added the 💪 phase/solved Post is done label Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🧑 semver/major This is a change 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

html value being turned into string
4 participants