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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(docs-infra): add canonical link to each adev page #56540

Closed
wants to merge 1 commit into from

Conversation

JeanMeche
Copy link
Member

No description provided.

@angular-robot angular-robot bot added area: docs Related to the documentation area: docs-infra Angular.dev application and infrastructure labels Jun 21, 2024
@ngbot ngbot bot added this to the Backlog milestone Jun 21, 2024
Copy link

github-actions bot commented Jun 21, 2024

Deployed adev-preview for c74e306 to: https://ng-dev-previews-fw--pr-angular-angular-56540-adev-prev-s1fcuqn6.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

adev/src/index.html Outdated Show resolved Hide resolved
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +21 to +23
const pathWithoutFragment = this.normalizePath(absolutePath).split('#')[0];
const fullPath = `${ANGULAR_DEV}/${pathWithoutFragment}`;
this.document.querySelector('link[rel=canonical]')?.setAttribute('href', fullPath);
Copy link

Choose a reason for hiding this comment

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

All the needed information can already be found with location.origin and location.pathname, both available on CSR/SSR.

Shouldn't it be enough to do as suggested? (this mean that you no longer need normalizePath)

This also rely on the real origin of the page instead of the hardcoded value, meaning that the canonical url will always match the real url, maybe that's not what you want?

Example: https://next.angular.dev/ and so on will be used for the canonical.

If that's not what you want, you can change this.document.location.origin to ANGULAR_DEV.

Suggested change
const pathWithoutFragment = this.normalizePath(absolutePath).split('#')[0];
const fullPath = `${ANGULAR_DEV}/${pathWithoutFragment}`;
this.document.querySelector('link[rel=canonical]')?.setAttribute('href', fullPath);
const fullPath = this.document.location.origin + this.document.location.pathname;
this.document.querySelector('link[rel=canonical]')?.setAttribute('href', fullPath);

Copy link
Member Author

Choose a reason for hiding this comment

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

We're doing pre-rendering, the domain isn't known at build time afaik.

* Information about the deployment of this application.
*/
@Injectable({providedIn: 'root'})
export class HeaderService {
Copy link

@Yberion Yberion Jun 22, 2024

Choose a reason for hiding this comment

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

nit: HeaderService feels a bit "generic", it could be header in term of html (h1, h2, etc), header in term of some header component, ....

What do you think about something a bit more specific like HtmlHeadService or something like that?

@JeanMeche JeanMeche added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jun 24, 2024
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit e81abdb.

The changes were merged into the following branches: main, 18.0.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker adev: preview area: docs Related to the documentation area: docs-infra Angular.dev application and infrastructure target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants