Skip to content

feat(schematics): replace href with routerLink #19341

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

Closed
wants to merge 1 commit into from
Closed

feat(schematics): replace href with routerLink #19341

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ...tion/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.html.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
[opened]="(isHandset$ | async) === false">
<mat-toolbar>Menu</mat-toolbar>
<mat-nav-list>
<a mat-list-item href="#">Link 1</a>
<a mat-list-item href="#">Link 2</a>
<a mat-list-item href="#">Link 3</a>
<a mat-list-item routerLink="/">Link 1</a>
<a mat-list-item routerLink="/">Link 2</a>
<a mat-list-item routerLink="/">Link 3</a>
Copy link
Member

Choose a reason for hiding this comment

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

I think we cannot do that as the schematic does not necessarily require @angular/router to be installed and set up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the question is are we going to use href, the schematic will not throw an exception even if we make this change. And I replace it with routerLink every time

Copy link
Member

@devversion devversion May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure. My thinking is that the schematic intends to provide a basic foundation for a navigation and that it's not obvious whether the Angular router is used or not. Adding routerLink is specific to the Angular router (even if it won't cause any type check error), and this might not be desired in all situations.

I definitely see the use-case for this though. We could have this as a prompt in the schematic, but then we probably should also ensure that @angular/router is installed, and also add RouterModule. Not sure if that is in scope for the schematic.

Copy link
Contributor Author

@santoshyadavdev santoshyadavdev May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We can have routerLink default and move to href based on input, i don't have stats, but i am sure most of the time, developers will use routerLink.

Copy link
Member

@devversion devversion May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I definitely agree that routerLink is commonly used for navigation, but my point is that routerLink does not work if @angular/router and the RouterModule are not set up.

It might not fail having a routerLink attribute, but the href is not set if the router module is not set up. Schematics should be helpful for beginners too, and leaving the code incomplete w/ needed additional manual action from users seems wrong.

The anchors will actually not be clickable if the module is not set up. So ideally, we'd ensure @angular/router is set up, and setup the RouterModule if needed.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @devversion that we can't assume that people will be using the router. If you go through ng new, you have to explicitly select that you want the router to be included.

</mat-nav-list>
</mat-sidenav>
<mat-sidenav-content>
Expand Down