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

[Doc Bug] Bad link from Fairlearn logo #1134

Closed
riedgar-ms opened this issue Oct 13, 2022 · 9 comments · Fixed by #1152
Closed

[Doc Bug] Bad link from Fairlearn logo #1134

riedgar-ms opened this issue Oct 13, 2022 · 9 comments · Fixed by #1152
Labels
bug Something isn't working documentation

Comments

@riedgar-ms
Copy link
Member

Describe the bug

Going to the 'main' branch of the documentation, the Fairlearn logo has a bad link

Steps/Code to Reproduce

image

This image has a link target of:

https://fairlearn.org/main/https%3A//fairlearn.org.html

Expected Results

Actual Results

Screenshots

Versions

@riedgar-ms riedgar-ms added bug Something isn't working documentation labels Oct 13, 2022
@riedgar-ms
Copy link
Member Author

I suspect that this is related to the logo link in html_theme_options in conf.py but I'm not sure exactly what is happening.

@iofall
Copy link
Contributor

iofall commented Oct 27, 2022

I can help.

If you look at the code for navbar-logo.html v0.9.0

{% if not theme_logo.get("link") %}
  {% set href = pathto(root_doc) %}
{% elif theme_logo.get("link").startswith("http") %}
  {# Logo link is to external URL}
  {% set href = theme_logo.get("link") %}
{% else %}
  {# Logo link is to internal page #}
  {% set href = pathto(theme_logo.get("link")) %}
{% endif %}

The line {# Logo link is to external URL} is missing the ending # thus making it an invalid comment and we directly jump into the last case, which gives us the wrong URL.

They have fixed it in the latest version - pydata/pydata-sphinx-theme#921

We can thus either upgrade to the latest version or just override the template with required fix.

@riedgar-ms
Copy link
Member Author

I wouldn't object to updating to the latest version of the theme. @adrinjalali do you have any concerns, since you were the last person tangling with the doc build?

@riedgar-ms
Copy link
Member Author

Thanks for the detective work @iofall !

@adrinjalali
Copy link
Member

It's complicated, we have been doing a branch per release. We can have a PR to that branch for the fix, but hopefully with the coming release it wouldn't be as urgent.

I don't mind having it fixed on that branch though. I just didn't want to push to all those branches.

@iofall
Copy link
Contributor

iofall commented Nov 9, 2022

Now that the release is done, we can fix this by:

  1. Fixing it on main
  2. And just updating the link manually on the docs repo for v8.0

What do you think?

@riedgar-ms
Copy link
Member Author

I'd be happy to see this fixed on main. We could even cherry pick to the release branch

@iofall
Copy link
Contributor

iofall commented Nov 9, 2022

Ok great, working on it.

But I don't think we need it on the release branch, right?

The docs will now remain the same for v8.0, so I was thinking of making a one-off commit to just manually change the link there. Because even if we change it on the release branch, we will still have to manually trigger the doc build for v8.0 (as far as I know).

@adrinjalali
Copy link
Member

Any commits to the release branch should automatically trigger a doc build and push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants