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

Switch the PHP client to Asciidoctor #690

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Mar 8, 2019

Switches the PHP client's doc to being built from Asciidoctor. The
resulting html has quite a few changes spacing changes that aren't
impactful and a couple of small changes that look fine visually. There
are a few changes to auto-generated IDs that we've grown to expect from
asciidoctor. They seem fine to me. Here is the diff:
https://gist.github.com/nik9000/72113b441f682b116b7b41a2b78baed6

Switches the PHP client's doc to being built from Asciidoctor. The
resulting html has quite a few changes spacing changes that aren't
impactful and a couple of small changes that look fine visually. There
are a few changes to auto-generated IDs that we've grown to expect from
asciidoctor. They seem fine to me. Here is the diff:
https://gist.github.com/nik9000/72113b441f682b116b7b41a2b78baed6
@nik9000 nik9000 requested review from polyfractal and lcawl March 8, 2019 19:55
@nik9000 nik9000 mentioned this pull request Mar 8, 2019
Copy link

@polyfractal polyfractal left a comment

Choose a reason for hiding this comment

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

I'll trust your judgment here and say 👍 :)

@nik9000 nik9000 merged commit 41ee072 into elastic:master Mar 8, 2019
@nik9000
Copy link
Member Author

nik9000 commented Mar 8, 2019

I'll trust your judgment here and say +1 :)

Thanks! A few minutes after the build is done you should see the changes in the site!

@lcawl
Copy link
Contributor

lcawl commented Mar 8, 2019

@polyfractal The changes seemed fine to me too. However, I think it is not a great idea long-term to rely on auto-generated IDs for all of the top pages in the book (e.g. the "_overview" in https://www.elastic.co/guide/en/elasticsearch/client/php-api/master/_overview.html). I'm not up-to-speed on how the asciidoc are generated for this client, but can we consider explicitly adding IDs (e.g. [[overview]]) in the files?

@nik9000
Copy link
Member Author

nik9000 commented Mar 8, 2019

can we consider explicitly adding IDs (e.g. [[overview]]) in the files?

I think that'd be a good idea, yeah. I'm fairly sure we didn't end up changing any of the urls when we switched to Asciidoctor, just the IDs on the page, luckily.

@nik9000
Copy link
Member Author

nik9000 commented Mar 8, 2019

@polyfractal an important thing: you'll need to use ./build_docs --asciidoctor when you build the book locally from here on out. This change updates the bash aliases file in the docs repo to tell you that, but I figure it is worth calling out specifically.

@polyfractal
Copy link

Noted, thanks for the headsup!

And having explicit IDs for pages seems like a good idea to me too.

@lcawl
Copy link
Contributor

lcawl commented Mar 8, 2019

Cool, I'll do a quick PR before I forget. Perhaps we can add redirects via your new process too, @nik9000

@nik9000
Copy link
Member Author

nik9000 commented Mar 8, 2019

Perhaps we can add redirects via your new process too, @nik9000

I've merged the file with the redirects in it but no one has picked it up yet. And I haven't built the spiffy "easy mode" redirects yet at all. But I expect we'll be able to stuff the new redirects into the docs repo sometime next week and be pretty confident about them.

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.

3 participants