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

Make x-pack tags clickable #744

Merged
merged 8 commits into from
Mar 27, 2019
Merged

Make x-pack tags clickable #744

merged 8 commits into from
Mar 27, 2019

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Mar 22, 2019

This replaces the x-pack tags on the titles of pages that are about
elastic licensed features with clickable links that take you to the
subscriptions page.

This is implemented by replacing the section rendering code in docbook
with customized code that includes the link.

This also replaces the background image of the x-pack tag with one
that looks "more clickable" and has a hover treatment.

Closes #720

This replaces the `x-pack` tags on the titles of pages that are about
elastic licensed features with clickable links that take you to the
subscriptions page.

This is implemented by replacing the section rendering code in docbook
with customized code that includes the link.

This also replaces the background image of the `x-pack` tag with one
that looks "more clickable" and has a hover treatment.

Closes elastic#720
@nik9000
Copy link
Member Author

nik9000 commented Mar 22, 2019

It used to look like:
old

Now it looks like when you don't hover:
new

And like this when you do hover:
new_hover

@nik9000
Copy link
Member Author

nik9000 commented Mar 22, 2019

I'm leaving this in "draft" state while I write a test for it.

@nik9000
Copy link
Member Author

nik9000 commented Mar 22, 2019

Ok - this works for sections that have role=xpack but it won't work for chapters with it. I'll see if we have any of those....

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.

It looks like I will certainly need to do chapters.

@@ -130,14 +99,6 @@
font-weight: inherit;
}

/* Add xpack icon to floating role=xpack titles in On This Page
Copy link
Member Author

Choose a reason for hiding this comment

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

These weren't used.

#guide div[class~="xpack"] > div > div > div > p > strong:after,

/* Add xpack icon to role=xpack dt titles */
#guide dt > span > span.xpack:after,
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we have any definitions that are just x-pack? If so I'll need to do something for them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't find any in the stack-docs, kibana, or elasticsearch repos

@nik9000 nik9000 marked this pull request as ready for review March 22, 2019 22:08
@nik9000
Copy link
Member Author

nik9000 commented Mar 22, 2019

OK! I've done the chapters too.

Copy link
Contributor

@jarpy jarpy left a comment

Choose a reason for hiding this comment

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

Fancy!

@lcawl
Copy link
Contributor

lcawl commented Mar 25, 2019

When I test this with the docbldso (Stack Overview) doc build, it fails to create an icon for this page:
https://github.com/elastic/stack-docs/edit/master/docs/en/stack/security/index.asciidoc (https://www.elastic.co/guide/en/elastic-stack-overview/master/elasticsearch-security.html)
Ditto for all the other pages at that level (that I tested).

@nik9000
Copy link
Member Author

nik9000 commented Mar 25, 2019

When I test this with the docbldso (Stack Overview) doc build, it fails to create an icon for this page:
https://github.com/elastic/stack-docs/edit/master/docs/en/stack/security/index.asciidoc (https://www.elastic.co/guide/en/elastic-stack-overview/master/elasticsearch-security.html)
Ditto for all the other pages at that level (that I tested).

Ooooh! I'll have a look at that then!

@nik9000
Copy link
Member Author

nik9000 commented Mar 25, 2019

Ooooh! I'll have a look at that then!

Done! @lcawl, could you have another look around?

@lcawl
Copy link
Contributor

lcawl commented Mar 25, 2019

The icons are appearing in the right places now, thanks!
I noticed, however, that they don't take me to a valid URL: file:///subscriptions
Is this something that's a known issue in local builds and will work when it's on the web?

@nik9000
Copy link
Member Author

nik9000 commented Mar 25, 2019

I noticed, however, that they don't take me to a valid URL: file:///subscriptions

I send you to /subscriptions because I figured that it'd be nicer to make a relative link. I can point them to https://www.elastic.co/subscriptions which'll work locally. I tend to try and avoid these sorts of links because they can lead you from a test environment to a production environment without you knowing it.

@lcawl
Copy link
Contributor

lcawl commented Mar 25, 2019

It seems to be encountering some problems in the case of sub-sections too:

image

The right hand window is the current (desired) formatting (https://www.elastic.co/guide/en/kibana/master/working-remote-clusters.html#managing-cross-cluster-replication) and the left hand window is built with this PR. Note the lack of icon and the reduced size of the title.

The same problem occurs in the Elasticsearch Reference for this X-Pack-tagged section: https://www.elastic.co/guide/en/elasticsearch/reference/master/modules-node.html#modules-node-xpack

@nik9000
Copy link
Member Author

nik9000 commented Mar 25, 2019

I hit that kind of thing over and over and over again while working on it. Let me see about this instance of it....

@nik9000
Copy link
Member Author

nik9000 commented Mar 26, 2019

Another thing that I notice is that I add the tag even when the section "above" it also has the flag. This seems wrong but something I can work out.

@nik9000
Copy link
Member Author

nik9000 commented Mar 26, 2019

@lcawl could you have another scan on this one when you have a chance? I've gotten further I think. And we'll end up with a nice set of tests for when we expect this and when we don't....

Copy link
Contributor

@lcawl lcawl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I tested Stack Overview, Kibana Reference, and Elasticsearch Reference.

@nik9000 nik9000 merged commit 26da66a into elastic:master Mar 27, 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.

3 participants