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

docs: invalid HammerLoader link reference #55836

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

docs: invalid HammerLoader link reference #55836

wants to merge 1 commit into from

Conversation

bougwal
Copy link
Contributor

@bougwal bougwal commented May 16, 2024

Fixes missing @ see to properly link to HammerLoader from Injection Token section

Screenshot 2024-05-16 at 22 37 31

@bougwal bougwal changed the title fix(docs): invalid HammerLoader link reference fix(docs-infra): invalid HammerLoader link reference May 16, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 16, 2024 •
edited

Deployed adev-preview for 3adaddb to: https://ng-dev-previews-fw--pr-angular-angular-55836-adev-prev-a1iofovk.web.app

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

JeanMeche
JeanMeche reviewed May 16, 2024
View reviewed changes
packages/platform-browser/src/dom/events/hammer_gestures.ts Outdated
@@ -78,7 +78,7 @@ export const HAMMER_GESTURE_CONFIG = new InjectionToken<HammerGestureConfig>('Ha
export type HammerLoader = () => Promise<void>;

/**
* Injection token used to provide a {@link HammerLoader} to Angular.
* Injection token used to provide a @see {@link HammerLoader} to Angular.
Copy link
Member

Choose a reason for hiding this comment

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

This creates a "pill" link. https://ng-dev-previews-fw--pr-angular-angular-55836-adev-prev-a1iofovk.web.app/api/platform-browser/HAMMER_LOADER

I think we should fix that when generating the template over on the dev-infra repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes right, it looked a bit strange from a UX standpoint (but functional)- Also adding that @ see would omit the text after the referenced link. In the deployed version the "to Angular" last part of the sentence is now omitted. Maybe a rephrase would be better now to put the link last and look at both issues ? wdyt @JeanMeche ?

Copy link
Member

Choose a reason for hiding this comment

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

Our template generator should be able to handle the link as is. I'd consider the curent behavior a bug.

What we could do is removing the link a go with a separate pill :

 * Injection token used to provide a HammerLoader to Angular.
 *
 * @see {@link HammerLoader}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-05-16 at 23 20 31

@bougwal bougwal force-pushed the fix/docs-hammer-loader branch 2 times, most recently from 5a4fe07 to a091e6d Compare May 16, 2024 21:24
@JeanMeche
Copy link
Member

Could you also make it a docs: ... commit message (no need for a scope), the docs-infra scope is meant for change on the adev app itself. Thank you.

docs: broken HammerLoader reference fix
3adaddb 
*Fixing broken documentation reference to HammerLoader in HammerLoaderInjectionToken page
@bougwal bougwal changed the title fix(docs-infra): invalid HammerLoader link reference docs: invalid HammerLoader link reference May 16, 2024
@angular-robot angular-robot bot added the area: docs Related to the documentation label May 16, 2024
@ngbot ngbot bot added this to the Backlog milestone May 16, 2024
@bougwal bougwal mentioned this pull request May 16, 2024
Open
@JeanMeche
Copy link
Member

Note: We'll wait for angular/dev-infra#2069 to be merge to likely amend this PR.

@JeanMeche
Copy link
Member

I've the dev-infra update in #55888 but it turns out there is a bug in the typescript parsing, so we're stuck with an upstream issue. See

@bencodezen bencodezen added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels May 20, 2024
@pkozlowski-opensource
Copy link
Member

@JeanMeche @bencodezen is this one ready to merge? Looking at the comments history I'm getting conflicting messages: it is ready for merge but also there is a comment from @JeanMeche saying that this is blocked: #55836 (comment)

@JeanMeche JeanMeche removed the action: merge The PR is ready for merge by the caretaker label May 23, 2024
JeanMeche
JeanMeche requested changes May 23, 2024
View reviewed changes
Copy link
Member

@JeanMeche JeanMeche left a comment

Choose a reason for hiding this comment

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

We're waiting to land another fix

@pkozlowski-opensource pkozlowski-opensource added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeanMeche JeanMeche JeanMeche requested changes

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews adev: preview area: docs Related to the documentation target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bougwal @JeanMeche @pkozlowski-opensource @bencodezen