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

HTML Reader: be more forgiving about figcaption #4184

Merged
merged 1 commit into from
Dec 27, 2017

Conversation

mb21
Copy link
Collaborator

@mb21 mb21 commented Dec 21, 2017

fixes #4183

@mb21 mb21 force-pushed the html-reader-figcaption branch from 88d4331 to 893fef2 Compare December 21, 2017 14:01
@mb21 mb21 changed the title WIP: HTML Reader: be more forgiving about figcaption HTML Reader: be more forgiving about figcaption Dec 21, 2017
skipMany pBlank
bs <- pInTags "figcaption" block
skipMany pBlank
let getInlines (Plain xs) = xs
Copy link
Owner

Choose a reason for hiding this comment

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

Couldn't we just use blocksToInlines from Text.Pandoc.Shared?

return $ B.para $ B.imageWith attr url ("fig:" ++ tit) caption
Just [Image attr alt (url, tit)] ->
let caption = fromMaybe alt mbcap
in return $ B.para $ B.imageWith attr url ("fig:" ++ tit) $ B.fromList caption
Copy link
Owner

Choose a reason for hiding this comment

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

As noted in the linked issue, I'm not convinced we should do this. The HTML doesn't have a caption, so why should we produce an image with a caption? The tricky thing is that we're overloading alt; it will still be used for the image's alt text, which just disappears in the current setup. I'm not for the alt attribute disappearing, but I'm also hesitant about creating a caption where there wasn't one before.

@mb21
Copy link
Collaborator Author

mb21 commented Dec 22, 2017

Thanks for the feedback, I agree.

One thing: Text.Pandoc.Shared should also export blocksToInlines', don't you think? The two places where blocksToInlines is already used, fromList is immediately called on it. Should I make another pull for that?

@jgm
Copy link
Owner

jgm commented Dec 22, 2017 via email

@mb21 mb21 force-pushed the html-reader-figcaption branch from 893fef2 to 9b54b94 Compare December 23, 2017 08:42
@mb21
Copy link
Collaborator Author

mb21 commented Dec 27, 2017

btw, I've rebased this and changed according to your comments.

@@ -588,8 +588,11 @@ pFigure = try $ do
skipMany pBlank
let pImg = (\x -> (Just x, Nothing)) <$>
(pOptInTag "p" pImage <* skipMany pBlank)
pCapt = (\x -> (Nothing, Just x)) <$>
(pInTags "figcaption" inline <* skipMany pBlank)
pCapt = (\x -> (Nothing, Just x)) <$> do
Copy link
Owner

Choose a reason for hiding this comment

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

You probably want to put a try around this one.
Otherwise it may fail after consuming some blanks, if there's no figcaption.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, do you need the skipMany pBlank? Won't this be handled by pSkip?

@jgm jgm merged commit acfa846 into jgm:master Dec 27, 2017
jgm added a commit that referenced this pull request Dec 27, 2017
@mb21
Copy link
Collaborator Author

mb21 commented Dec 27, 2017

right... thanks!

@mb21 mb21 deleted the html-reader-figcaption branch December 27, 2017 23:10
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.

HTML Reader: Regression for figure without figcaption
2 participants