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

feat: adds option to open aux links in new tab #229

Merged
merged 9 commits into from
Apr 27, 2020
Merged

feat: adds option to open aux links in new tab #229

merged 9 commits into from
Apr 27, 2020

Conversation

mattxwang
Copy link
Member

@mattxwang mattxwang commented Oct 2, 2019

Hey! This is a simple PR that implements the feature requested in #165, namely some way to make aux links open in a new tab.

  • I added a key to _config.yml called aux_links_new_tab, and if it's set to true, aux links will open in a new tab via target="_blank"
  • I also set rel="noopener noreferrer" for security reasons
  • aux_links_new_tab is set to false by default to maintain the previous behaviour before this PR

Let me know what you think! I'm not super proud of the semi-janky if statement in the header, and if there's a better way to implement it I'm happy to change it.

closes #165

Comment on lines 47 to 51
{% if site.aux_links_new_tab %}
<a href="{{ link.last }}" target="_blank" rel="noopener noreferrer">
{% else %}
<a href="{{ link.last }}">
{% endif %}

Choose a reason for hiding this comment

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

You could write it like this:

<a href="{{ link.last }}"{% if site.aux_links_new_tab != false %} target="_blank" rel="noopener noreferrer"{% endif %}>

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think that's a bit cleaner. Updated the branch!

@svoegeli
Copy link

Would anyone be available to review and merge this PR? It would be very useful for a project I'm working on. Thanks!

@mattxwang
Copy link
Member Author

Would anyone be available to review and merge this PR? It would be very useful for a project I'm working on. Thanks!

I think @pmarsceill's been pretty busy w/ other work, but doesn't hurt to ping him again. If it's super critical, you can download the fork and use the theme directly from there!

@pmarsceill pmarsceill added the next-minor-release next minor release bump label Apr 27, 2020
@pmarsceill pmarsceill changed the base branch from master to v0.2.9 April 27, 2020 19:57
@pmarsceill
Copy link
Collaborator

Ignore the failing CI check, I have to fix how GH Actions implements the formatting changes from Prettier.

@mattxwang
Copy link
Member Author

mattxwang commented Apr 27, 2020

Haha, no worries, just been periodically updating the branch to match base.

@pmarsceill
Copy link
Collaborator

This is a great addition -- only thing is maybe we should add an icon like: https://feathericons.com/?query=external-link if the feature is enabled?

@mattxwang
Copy link
Member Author

This is a great addition -- only thing is maybe we should add an icon like: https://feathericons.com/?query=external-link if the feature is enabled?

Sure, is feather icons the normal icon set that's used for just-the-docs? I can probably implement that pretty quickly!

@mattxwang
Copy link
Member Author

Actually, on second thought, isn't an icon like that normally used just to indicate that a link is external? I'm not sure if this option should change that behaviour. Also a bit worried about visual clutter in the nav?

@pmarsceill
Copy link
Collaborator

Actually, on second thought, isn't an icon like that normally used just to indicate that a link is external? I'm not sure if this option should change that behaviour. Also a bit worried about visual clutter in the nav?

Yeah you're probably right. I think this is good as-is.

@pmarsceill
Copy link
Collaborator

I'm going to merge into the 2.9 branch. Thanks!

@pmarsceill pmarsceill merged commit b4c278c into just-the-docs:v0.2.9 Apr 27, 2020
@pmarsceill pmarsceill mentioned this pull request Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-minor-release next minor release bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants