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

fix(gatsby-transformer-remark): fix excerpt generation - strip excessive white spaces, extract alt from images #12878

Merged
merged 12 commits into from
Apr 25, 2019

Conversation

macklinu
Copy link
Contributor

Description

This is a failing test case to reproduce the issue stated in #12398. Here is a screenshot of test failure output:

image

Related Issues

Relates to #12398

@wardpeet
Copy link
Contributor

@macklinu mind fixing this as well? Thanks for creating the test, if you don't really know how we can take over from here.

@wardpeet wardpeet added the status: awaiting author response Additional information has been requested from the author label Mar 27, 2019
@macklinu
Copy link
Contributor Author

@wardpeet I'll take a look at patching this and ping you if I come up with a solution or need to ask for another contributor's help. Thank you!

@macklinu
Copy link
Contributor Author

Would it make sense to use strip-markdown as a remark plugin instead of writing our own markdown processor in order to parse out the excerpt content? Or maybe we don't need all the functionality of strip-markdown and just need to account for a few more node types? Any implementation suggestions?

async function getExcerpt(
markdownNode,
{ format, pruneLength, truncate, excerptSeparator }
) {
if (format === `html`) {
const excerptAST = await getExcerptAst(markdownNode, {
pruneLength,
truncate,
excerptSeparator,
})
const html = hastToHTML(excerptAST, {
allowDangerousHTML: true,
})
return html
}
if (markdownNode.excerpt) {
return markdownNode.excerpt
}
const text = await getAST(markdownNode).then(ast => {
const excerptNodes = []
visit(ast, node => {
if (node.type === `text` || node.type === `inlineCode`) {
excerptNodes.push(node.value)
}
return
})
if (!truncate) {
return prune(excerptNodes.join(` `), pruneLength, `…`)
}
return _.truncate(excerptNodes.join(` `), {
length: pruneLength,
omission: `…`,
})
})
return text
}

@wardpeet
Copy link
Contributor

wardpeet commented Mar 28, 2019

strip-markdown strips markdown away, we still want to show the link or bold text right? That plugin just makes the text unstyled.

So if not mistaken it's not really what we're after

@macklinu
Copy link
Contributor Author

Ah, I see. I think my failing test assertion is incorrect then. We don't want to necessarily strip markdown, but we just want to preserve markdown as is and not remove any content?

@wardpeet
Copy link
Contributor

yes, the issue is that an extra space is added between the link & the ! symbol. It might be something that we have to fix upstream.

@frinyvonnick
Copy link
Contributor

frinyvonnick commented Apr 23, 2019

Hi @macklinu 👋,

First thanks for your failing test it really helped me diving in this bug 👏

I'm not sure to understood all you said with @wardpeet.

I found out that the function getExcerpt in extend-node-type.js wasn't handling image and the last link was classified as an image and in contrary of links image doesn't have a text node in it. That was causing the missing parts. The second problem of extra spaces was caused by a join of excerpt parts with a space.

Was that the right thing to do ?

Sorry to not asking before pushing on your branch 😊

@frinyvonnick frinyvonnick force-pushed the issue-12398/failing-test branch 2 times, most recently from 5fbeb59 to 8859d42 Compare April 23, 2019 18:12
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Nice! Just one nitpicky comment/question about value of snapshot in the test (as most value comes from node.excerpt assertion)

@pieh pieh changed the title test(transformer-remark): add failing test case Apr 24, 2019
@frinyvonnick
Copy link
Contributor

Thanks for your review @pieh 👍

@wardpeet
Copy link
Contributor

Sweet! thanks for getting this PR through the door @frinyvonnick! I'll fix the unit test.

@frinyvonnick
Copy link
Contributor

Thanks @wardpeet 🙏 !

@wardpeet wardpeet removed the status: awaiting author response Additional information has been requested from the author label Apr 25, 2019
@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Apr 25, 2019
@wardpeet
Copy link
Contributor

pretty sure we can ignore danger here.

@wardpeet wardpeet merged commit ceb0d72 into master Apr 25, 2019
@sidharthachatterjee sidharthachatterjee deleted the issue-12398/failing-test branch April 25, 2019 13:59
@sidharthachatterjee
Copy link
Contributor

Published in gatsby-transformer-remark@2.3.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
5 participants