-
-
Notifications
You must be signed in to change notification settings - Fork 785
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
Add canonical link #3872
Add canonical link #3872
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rebase on main and add the snpshots please? 🙏
0e0eeb0
to
8bde60f
Compare
between multiple versions of the same package. --> | ||
<script src="./docs_config.js"></script> | ||
<link id="syntax-theme" rel="stylesheet" href="./css/atom-one-light.min.css?v=GLEAM_VERSION_HERE"/> | ||
<link rel="canconical" href="./test_canconical/app.html" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Canonical URLs need to be absolute!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we know the absolute URL here? Since users could host it themselves; I am not aware of where this setting is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be set via configuration when publishing to Hex. We can omit otherwise for now
@@ -18,6 +18,7 @@ expression: "compile_with_markdown_pages(config, vec![], pages)" | |||
between multiple versions of the same package. --> | |||
<script src="./docs_config.js"></script> | |||
<link id="syntax-theme" rel="stylesheet" href="./css/atom-one-light.min.css?v=GLEAM_VERSION_HERE"/> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you configure all the tests to have them please 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not displayed because the project_name is blank
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I'm asking you to add it to all of them 🙏 Thank you.
A test case for rendering not-for-hex would also be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fab! Couple tiny notes inline. Could you update the changelog also please
{% if !host.is_empty() && !project_name.is_empty() -%} | ||
<link rel="canonical" href="{{ host }}/{{ project_name|safe }}/{{ file_path|safe }}" /> | ||
{%- endif %} | ||
{%- endblock %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to have this in the layout? It seems to be the same for both templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally was more direct to put it where I knew it was being used, but has converged enough that I now have moved it directly into the layout.
@@ -18,6 +18,7 @@ expression: "compile_with_markdown_pages(config, vec![], pages)" | |||
between multiple versions of the same package. --> | |||
<script src="./docs_config.js"></script> | |||
<link id="syntax-theme" rel="stylesheet" href="./css/atom-one-light.min.css?v=GLEAM_VERSION_HERE"/> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I'm asking you to add it to all of them 🙏 Thank you.
A test case for rendering not-for-hex would also be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful!! Thank you
<head>
There are snapshots as well but wanted to get this committed and reviewed.
Fixes #3864
Thanks.