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

Asciidoctor: Copy callout icons #570

Merged
merged 6 commits into from
Feb 4, 2019

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Jan 31, 2019

Adds support for copying callout icons into the directory containing the
built documents. Without this callout icons all 404 which is sad because
they are pretty.

Adds support for copying callout icons into the directory containing the
built documents. Without this callout icons all 404 which is sad because
they are pretty.
@nik9000 nik9000 requested a review from ddillinger January 31, 2019 21:20
@nik9000
Copy link
Member Author

nik9000 commented Jan 31, 2019

I've made an effort not to conflict with #565 but a little conflict is impossible to avoid I think.

@nik9000 nik9000 mentioned this pull request Jan 31, 2019
@nik9000
Copy link
Member Author

nik9000 commented Feb 4, 2019

This one'll need master merged into it and a few paths fixed up before merging.

@@ -92,7 +95,8 @@ sub build_chunked {
# missing attributes!
# '-a' => 'attribute-missing=warn',
'-a' => 'asciidoc-dir=' . $asciidoc_dir,
$resources ? ( '-a' => 'resources=' . join(',', @$resources)) : (),
'-a' => 'resources=' . join(',', @$resources),
'-a' => 'copy-callout-images=png',

Choose a reason for hiding this comment

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

Does this mean they'll always and forever have to be png? That may be totally fine, I just am asking to be sure that it is known and definitely totally fine :)

Copy link
Member Author

Choose a reason for hiding this comment

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

They are current pngs. We could change it here to whatever we have images for though.

@@ -107,7 +111,7 @@ sub build_chunked {
file('resources/website_chunked.xsl')->absolute,
"$dest/index.xml"
);
unlink "$dest/index.xml";
# unlink "$dest/index.xml";

Choose a reason for hiding this comment

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

Commented-out code left behind

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

copy_image block, uri
else
extension = block.attr 'copy-callout-images'
if block.parent && block.parent.context == :colist

Choose a reason for hiding this comment

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

This could be an elsif on line 24 right? There's no other in-scope usage of extension anyway here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! This is what that empty test block was about! It looks like I never finished it. sorry!

@@ -201,4 +201,130 @@ def copy_attributes copied
expect(copied).to eq([])
}
end

it "copies a images for callouts when requested (png)" do

Choose a reason for hiding this comment

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

typo: "copies a images"

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

])
end

it "copies a images for callouts when requested (gif)" do

Choose a reason for hiding this comment

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

typo: "copies a images"

])
end

it "has a nice error message when a callout image is missing" do

Choose a reason for hiding this comment

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

❤️ ❤️ ❤️

])
end

it "doesn't copy callout images if the extension isn't set" do

Choose a reason for hiding this comment

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

empty test body

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this! I never finished it. I'll get it soon!

@nik9000
Copy link
Member Author

nik9000 commented Feb 4, 2019

@ddillinger I pushed a few patches to finish this off. Thanks again for catching it!

@nik9000
Copy link
Member Author

nik9000 commented Feb 4, 2019

Thanks @ddillinger!

@nik9000 nik9000 merged commit 7fedf72 into elastic:master Feb 4, 2019
nik9000 added a commit that referenced this pull request Feb 15, 2019
Adds support for copying callout icons into the directory containing the
built documents. Without this callout icons all 404 which is sad because
they are pretty.
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.

2 participants