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 adminition that had '|| @caption' appended #142

Merged
merged 6 commits into from
Sep 28, 2017

Conversation

JacobAae
Copy link
Contributor

When making slides and using the admonition block, a "|| @caption" was appended. This changes the parenthesis, to where I think it should be, adding the caprion only if there is no textlabel

image

@mojavelinux
Copy link
Member

This is definitely a bug, though I'm not sure the fix has the desired result. I think it needs to be:

i class=%(fa fa-#{icon_mapping[attr :name]}) title=((attr :textlabel) || @caption)

Slim is clearly getting confused about precedence.

Another way to write it is:

title=(attr :textlabel, @caption)

though that has slightly different behavior.

@jirutka
Copy link
Member

jirutka commented Sep 21, 2017

i class=%(fa fa-#{icon_mapping[attr :name]}) title=(attr :textlabel) || @caption

Slim syntax is not so complicated, || @caption is interpreted as a text content of the element (see docs), that should not be surprising.

As you’ve figured out, you must wrap it in parenthesis: …title=((attr :textlabel) || @caption) (or …title=(attr :textlabel, @caption)), or in idiomatic Ruby syntax: …title=(attr(:textlabel) || @caption) (or …title=attr(:textlabel, @caption)).

@mojavelinux
Copy link
Member

mojavelinux commented Sep 21, 2017

Slim is clearly getting confused about precedence.

I say that only because it differs from Ruby rules. In Ruby, everything to the right of the equal sign is implicitly contained in round brackets.

To Slim's credit, we're not really talking about the same equals sign since this is an attribute list. So it should look more like an entry in a Hash (map) definition, which would require the brackets.

@obilodeau
Copy link
Member

I've tracked the regression to this commit: e6344890#diff-a550ba4d478c3d43dff573f7d35df4de.

I thought the PR was missing a fix (the other || @caption introduced) so I generated test cases. Turns out this PR solves the only problematic case.

Before I merge it however, I would like #131 to go in since there's a folder re-org in there and I would have to rebase #131 again which I would rather avoid.

@obilodeau
Copy link
Member

Can you rebase your pull request please?

Basically, templates/slim/block_admonition.html.slim should now be templates/admonition.html.slim, test/output/slim/admonitions-icons.html should now be test/doctest/admonitions-icons.html and test/output/slim/admonitions.html should now be test/doctest/admonitions.html.

Thanks!

@obilodeau obilodeau added this to the 1.1.0 milestone Sep 28, 2017
@obilodeau
Copy link
Member

👍

@obilodeau obilodeau merged commit 403e859 into asciidoctor:master Sep 28, 2017
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