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.

Sign up for GitHub

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

Jump to bottom

Only show authors' email addresses if so configured in git-authors #6494

Merged
merged 1 commit into from
Dec 12, 2023
Merged

Only show authors' email addresses if so configured in git-authors #6494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions material/templates/partials/source-file.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
</span>
{% endmacro %}
{% macro render_authors(authors) %}
{% set git_authors = config.plugins.get("git-authors") %}
<span class="md-source-file__fact">
<span class="md-icon" title="{{ lang.t('source.file.contributors') }}">
{% if authors | length == 1 %}
Expand All @@ -28,9 +29,13 @@
</span>
<nav>
{% for author in authors %}
{%- if git_authors.config.show_email_address %}
<a href="mailto:{{ author.email }}">
{{- author.name -}}
</a>
{%- else -%}
{{- author.name -}}
{% endif -%}
{%- if loop.revindex > 1 %}, {% endif -%}
{% endfor %}
</nav>
Expand Down
5 changes: 5 additions & 0 deletions src/templates/partials/source-file.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@

<!-- Render authors -->
{% macro render_authors(authors) %}
{% set git_authors = config.plugins.get("git-authors") %}
<span class="md-source-file__fact">
<span class="md-icon" title="{{ lang.t('source.file.contributors') }}">
{% if authors | length == 1 %}
Expand All @@ -54,9 +55,13 @@
</span>
<nav>
{% for author in authors %}
{%- if git_authors.config.show_email_address %}
Copy link
Owner

Choose a reason for hiding this comment

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

One more spot: We don't need whitespace control here. Please remove the - on all tags that you added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that include the one instance of {{- author.name -}} that I duplicated? In other words, should the hyphens be dropped from that one as well?

Copy link
Owner

@squidfunk squidfunk Dec 12, 2023 •
edited

Choose a reason for hiding this comment

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

Yes. The following line is the only line where this is necessary, because we want the , to stick to the author name, but this is a line you did not touch, which is why I meant to remove them from all lines you touched.

mkdocs-material/src/templates/partials/source-file.html

Line 60 in 2ca5cf0

{%- if loop.revindex > 1 %}, {% endif -%}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I am not so sure about that. If I drop all instances of - that I introduced here, then if show_email_address is true, I end up with

    <nav>
      
        
          <a href="mailto:john.doe@example.com">John Doe</a>
        , 
        
          <a href="mailto:jane.doe@example.com">Jane Doe</a>
        
    </nav>

And I'm pretty sure we don't want that comma on its own line, because it gets rendered as

John Doe , Jane Doe

... and the thus-appearing extra whitespace before the comma is extraneous.

As the patch is currently proposed, what we end up with is

    <nav>
        
          <a href="mailto:john.doe@example.com">John Doe</a>, 
          <a href="mailto:jane.doe@example.com">Jane Doe</a>
    </nav>

... which is what we want, I think. Would you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just pushed 7f0cdd2 which is the most minimal patch that I can think of, with no whitespace management added at all. But that introduces the extra space before the comma, no matter if we're rendering a link or not. So please do not merge that as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That fixes things only for show_email_address: false; but still breaks (i.e. introduces the extraneous whitespace) for the default of show_email_address: true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does the right thing, regardless of how show_email_address is set:

       {% for author in authors %}
+        {%- if git_authors.config.show_email_address %}
         <a href="mailto:{{ author.email }}">
           {{- author.name -}}
         </a>
+        {%- else -%}
+          {{- author.name -}}
+        {% endif -%}
         {%- if loop.revindex > 1 %}, {% endif -%}
       {% endfor %}

Okay if I update my topic branch with that?

Copy link
Owner

@squidfunk squidfunk Dec 12, 2023 •
edited

Choose a reason for hiding this comment

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

I'm sure you don't need all of those whitespace control characters, but okay, let's keep the whitespace control. I'll look into in the future. We should keep whitespace control to a minimum, and only use it where absolutely necessary.

Copy link
Contributor Author

@fghaas fghaas Dec 12, 2023 •
edited

Choose a reason for hiding this comment

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

I sympathise with that, but if you instruct me to "remove the - on all tags that [I] added" and simultaneously "we want the , to stick to the author name", I can only do one xor the other. :)

Copy link
Owner

Choose a reason for hiding this comment

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

Haha yeah, sorry for that. Entirely my fault. I'm sorry for the back and forth. Is it okay to merge now?

<a href="mailto:{{ author.email }}">
{{- author.name -}}
</a>
{%- else -%}
{{- author.name -}}
{% endif -%}
{%- if loop.revindex > 1 %}, {% endif -%}
{% endfor %}
</nav>
Expand Down