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

Simplify navbar #3681

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Simplify navbar #3681

wants to merge 6 commits into from

Conversation

tomchristie
Copy link
Member

Refs #3680

image
  • Drop next/prev controls.
  • Drop text from search & repo controls, except in expanded menu.
expanded menu image

Copy link
Sponsor Contributor

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Looks good, although the removal of the next_prev block probably is a breaking change?

@tomchristie
Copy link
Member Author

Good pointer, thanks.

I've re-included it as an empty block here and amended the docs marginally. ☺️

mkdocs/themes/mkdocs/base.html Outdated Show resolved Hide resolved
@HumbleDeer
Copy link

The removal of the "Edit on GitHub" text might obscure the purpose of the GitHub icon in the navbar.

If this simplification is still desirable, I suggest amending this by adding an "Edit on GitHub" link at the bottom of a page instead, as is custom with a fair few other static site generator packages and other documentation generators alike.

@tomchristie
Copy link
Member Author

Good catch. Tho we should switch this up completely and just have a repo_url control here, rather than page.edit_url.

If we want to keep an "Edit this page" in the default theme, then that control should be in the page area not the nav bar area.

</li>
{%- endif %}
{%- endblock %}
{%- block next_prev %}{%- endblock %}

{%- block repo %}
{%- if page and page.edit_url %}
<li class="nav-item">
<a href="{{ page.edit_url }}" class="nav-link">
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
<a href="{{ page.edit_url }}" class="nav-link">
<a href="{{ config.repo_url }}" class="nav-link">

{%- elif config.repo_name == 'GitLab' -%}
<i class="fa-brands fa-gitlab"></i> {% trans repo_name=config.repo_name %}Edit on {{ repo_name }}{% endtrans %}
<i class="fa-brands fa-gitlab"></i><span class="d-lg-none ms-2">{% trans repo_name=config.repo_name %}Edit on {{ repo_name }}{% endtrans %}</span>
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
<i class="fa-brands fa-gitlab"></i><span class="d-lg-none ms-2">{% trans repo_name=config.repo_name %}Edit on {{ repo_name }}{% endtrans %}</span>
<i class="fa-brands fa-gitlab"></i><span class="d-lg-none ms-2">{{ config.repo_name }}</span>

{%- elif config.repo_name == 'Bitbucket' -%}
<i class="fa-brands fa-bitbucket"></i> {% trans repo_name=config.repo_name %}Edit on {{ repo_name }}{% endtrans %}
<i class="fa-brands fa-bitbucket"></i><span class="d-lg-none ms-2">{% trans repo_name=config.repo_name %}Edit on {{ repo_name }}{% endtrans %}</span>
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
<i class="fa-brands fa-bitbucket"></i><span class="d-lg-none ms-2">{% trans repo_name=config.repo_name %}Edit on {{ repo_name }}{% endtrans %}</span>
<i class="fa-brands fa-bitbucket"></i><span class="d-lg-none ms-2">{{ config.repo_name }}</span>


{%- block repo %}
{%- if page and page.edit_url %}
<li class="nav-item">
<a href="{{ page.edit_url }}" class="nav-link">
{%- if config.repo_name == 'GitHub' -%}
<i class="fa-brands fa-github"></i> {% trans repo_name=config.repo_name %}Edit on {{ repo_name }}{% endtrans %}
<i class="fa-brands fa-github"></i><span class="d-lg-none ms-2">{% trans repo_name=config.repo_name %}Edit on {{ repo_name }}{% endtrans %}</span>
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
<i class="fa-brands fa-github"></i><span class="d-lg-none ms-2">{% trans repo_name=config.repo_name %}Edit on {{ repo_name }}{% endtrans %}</span>
<i class="fa-brands fa-github"></i><span class="d-lg-none ms-2">{{ config.repo_name }}</span>

Comment on lines 135 to 136
{%- elif config.repo_name -%}
{% trans repo_name=config.repo_name%}Edit on {{ repo_name }}{% endtrans %}
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
{%- elif config.repo_name -%}
{% trans repo_name=config.repo_name%}Edit on {{ repo_name }}{% endtrans %}
{%- elif config.repo_name -%}
{{ config.repo_name }}

@HumbleDeer
Copy link

Good catch. Tho we should switch this up completely and just have a repo_url control here, rather than page.edit_url.

If we want to keep an "Edit this page" in the default theme, then that control should be in the page area not the nav bar area.

Other docs packages often have edit buttons in the page area (indeed), and have it optional or to be enabled manually. I think that's a solid option, and agree it should be a page area element.

In that sense (and I don't mean this snarkily), a navigation bar is for navigation. Anything not navigation or locating of pages like a navigation is possibly better off in the footer or page content.

@dagnelies
Copy link

dagnelies commented May 11, 2024

  • 👍 Drop next/prev controls.
  • 👍 Move "edit on Github" from the navbar to the page
  • 😢 Drop text from search controls

I think these are good choice, as it would make the navbar lighter ...except for the "search". Searching is so crucial that it deserves to be clearly visible IMHO, including the "Search" label, not only the small icon.

The prev/next/edit indeed better belong to the page than the navbar.

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.

None yet

4 participants