Skip to content

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

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
Merged
merged 1 commit into from
Jun 19, 2020
Merged

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

merged 1 commit into from
Jun 19, 2020

Conversation

santoshyadavdev
Copy link
Contributor

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

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
Copy link
Contributor

Relates to PR #19341.

crisbeto
crisbeto reviewed May 25, 2020
View reviewed changes
@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
Copy link
Contributor Author

Hi @Splaktar ,
This is ready for review now.

alan-agius4
alan-agius4 reviewed May 25, 2020
View reviewed changes
src/material/schematics/ng-generate/navigation/schema.json
@@ -92,6 +92,11 @@
"type": "boolean",
"default": false,
"description": "Specifies if the component is an entry component of declaring module."
},
"routing": {
Copy link
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.

Copy link
Member

@devversion devversion May 25, 2020 •
edited
Loading

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
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@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
jelbourn
jelbourn approved these changes Jun 16, 2020
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

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
Copy link
Contributor Author

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

@jelbourn
Copy link
Member

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

@santoshyadavdev
Copy link
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
Copy link
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
Copy link

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

@jelbourn jelbourn jelbourn approved these changes

@crisbeto crisbeto crisbeto approved these changes

@devversion devversion devversion approved these changes

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