Skip to content

Copy images from unordeed and definition lists #1093

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

Merged
merged 12 commits into from
Aug 28, 2019

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Aug 22, 2019

We had support for copying images within ordered lists but not within
unordered lists. This fixes that.

We had support for copying images within ordered lists but not within
*un*ordered lists. This fixes that.
@nik9000 nik9000 requested a review from haywoood August 22, 2019 20:55
@nik9000
Copy link
Member Author

nik9000 commented Aug 22, 2019

@haywoood, do you have time to look at this one?

@nik9000 nik9000 changed the title Copy images from unordeed lists Copy images from unordeed and definition lists Aug 22, 2019
@nik9000
Copy link
Member Author

nik9000 commented Aug 22, 2019

There is more to do here.....

@nik9000
Copy link
Member Author

nik9000 commented Aug 22, 2019

I think I've got it now!

Copy link
Contributor

@haywoood haywoood left a comment

Choose a reason for hiding this comment

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

🍔

@nik9000
Copy link
Member Author

nik9000 commented Aug 23, 2019

@elasticmachine, run elasticsearch-ci/docs rebuild

I want to make super sure this is ok on all the books, just in case.

@nik9000
Copy link
Member Author

nik9000 commented Aug 23, 2019

@elasticmachine, run elasticsearch-ci/docs rebuild

@nik9000
Copy link
Member Author

nik9000 commented Aug 23, 2019

The failure here is real! I'm tracking it down. Weird stuff.

@nik9000
Copy link
Member Author

nik9000 commented Aug 23, 2019

OK! I discovered the issue here. The mechanism that I was using to resolve attributes is wrong. I was replaying "my" attributes but if the attributes are set "far enough" from where they are used they aren't found. I'll have to rework a bunch of stuff to get this working....

@nik9000
Copy link
Member Author

nik9000 commented Aug 26, 2019

@elasticmachine, run elasticsearch-ci/docs rebuild

@nik9000
Copy link
Member Author

nik9000 commented Aug 27, 2019

OK! The solution that I have here doesn't crash and passes the tests I could come up with so it might be better than what we had. I'm going to go over the diff later and see if I can find any images it didn't copy.

Copy link
Member Author

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

@haywoood, I think this is ready! It is quite different, but it has to be to pick up images properly. I think this pattern will ultimately simplify how I plug in to Asciidoctor.

@@ -0,0 +1,30 @@
# frozen_string_literal: true
Copy link
Member Author

Choose a reason for hiding this comment

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

This class is the real magic in this PR: instead of plugging in to asciidoctor using its plugin mechanism this plugs in using the "converter". This allows us to plug whatever behavior we need in at render time when the attributes are properly resolved.

Copy link
Contributor

@haywoood haywoood left a comment

Choose a reason for hiding this comment

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

🍔

@nik9000
Copy link
Member Author

nik9000 commented Aug 27, 2019

@elasticmachine, run elasticsearch-ci/docs rebuild

@nik9000
Copy link
Member Author

nik9000 commented Aug 27, 2019

Just for extra paranoia....

@nik9000 nik9000 merged commit df82278 into elastic:master Aug 28, 2019
@nik9000
Copy link
Member Author

nik9000 commented Aug 28, 2019

🍟

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