Skip to content

feat(material/schematics/navigation): enable routing option - #19439

Merged
mmalerba merged 1 commit into
angular:masterfrom
santoshyadavdev:feat(schematics)--change-href-to-routerLink
Jun 19, 2020
Merged

feat(material/schematics/navigation): enable routing option#19439
mmalerba merged 1 commit into
angular:masterfrom
santoshyadavdev:feat(schematics)--change-href-to-routerLink

Conversation

@santoshyadavdev

@santoshyadavdev santoshyadavdev commented May 25, 2020 •
edited by Splaktar
Loading

Copy link
Copy Markdown
Contributor

Please do not review as of now

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 25, 2020
@Splaktar
Splaktar marked this pull request as draft May 25, 2020 07:57
@Splaktar

Splaktar commented May 25, 2020

Copy link
Copy Markdown
Contributor

Relates to PR #19341.

crisbeto
crisbeto reviewed May 25, 2020
View reviewed changes
Comment thread ...igation/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.html.template Outdated
Comment thread src/material/schematics/ng-generate/navigation/index.spec.ts Outdated
Comment thread src/material/schematics/ng-generate/navigation/schema.json Outdated
@santoshyadavdev
santoshyadavdev marked this pull request as ready for review May 25, 2020 15:21
@santoshyadavdev
santoshyadavdev requested a review from crisbeto May 25, 2020 15:22
@santoshyadavdev

santoshyadavdev commented May 25, 2020

Copy link
Copy Markdown
Contributor Author

Hi @Splaktar ,
This is ready for review now.

alan-agius4
alan-agius4 reviewed May 25, 2020
View reviewed changes
Comment thread src/material/schematics/ng-generate/navigation/schema.json
"default": false,
"description": "Specifies if the component is an entry component of declaring module."
},
"routing": {

@alan-agius4 alan-agius4 May 25, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I personally think that the schematic should be doing this check based on the app module and not provided as an option.

@devversion devversion May 25, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree that this would be the nicest solution. Though that might be overly complicated as we'd need to check the imports of the module.

These checks generally are not reliable as in reality the Angular compiler supports partial evaluation (i.e. module can be specified in a separate file through an exported variable). Surely we could use the static interpreter as in FW update migrations, but that seems out of scope.

@Splaktar Splaktar changed the title feat(schematics): enable routing option feat(material/schematics/navigation): enable routing option May 25, 2020
crisbeto
crisbeto approved these changes May 26, 2020
View reviewed changes

@crisbeto crisbeto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread ...igation/files/__path__/__name@dasherize@if-flat__/__name@dasherize__.component.html.template Outdated
@devversion devversion added lgtm target: patch This PR is targeted for the next patch release labels May 27, 2020
jelbourn
jelbourn reviewed May 28, 2020
View reviewed changes
Comment thread src/material/schematics/ng-generate/navigation/schema.json Outdated
jelbourn
jelbourn approved these changes Jun 16, 2020
View reviewed changes

@jelbourn jelbourn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release merge safe and removed target: patch This PR is targeted for the next patch release labels Jun 16, 2020
@santoshyadavdev

santoshyadavdev commented Jun 18, 2020

Copy link
Copy Markdown
Contributor Author

Hi @jelbourn ,
Do I need to update commit message?

@jelbourn

jelbourn commented Jun 18, 2020

Copy link
Copy Markdown
Contributor

It would help if it was just feat(material/schematics), but we can also change it when we merge

@santoshyadavdev

santoshyadavdev commented Jun 18, 2020

Copy link
Copy Markdown
Contributor Author

It would help if it was just feat(material/schematics), but we can also change it when we merge

Let me do it, would be quick anyways.

@santoshyadavdev

santoshyadavdev commented Jun 18, 2020

Copy link
Copy Markdown
Contributor Author

It would help if it was just feat(material/schematics), but we can also change it when we merge

Done.

@mmalerba
mmalerba merged commit 31723b8 into angular:master Jun 19, 2020
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Jul 20, 2020

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Jul 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@alan-agius4 alan-agius4 alan-agius4 left review comments
@crisbeto crisbeto crisbeto approved these changes
@devversion devversion devversion approved these changes
+1 more reviewer
@jelbourn jelbourn jelbourn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@santoshyadavdev @Splaktar @jelbourn @crisbeto @devversion @alan-agius4 @googlebot @mmalerba