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

feat(material/schematics): add the scaffolding for an mdc-migration schematic #23804

Merged
merged 2 commits into from Oct 28, 2021
Merged

feat(material/schematics): add the scaffolding for an mdc-migration schematic #23804

merged 2 commits into from Oct 28, 2021

Conversation

mmalerba
Copy link
Contributor

also adds an app to test it on

@mmalerba mmalerba requested review from jelbourn and a team as code owners October 21, 2021 00:39
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 21, 2021
@mmalerba mmalerba added the target: feature This PR is targeted for a feature branch (outside of main and semver branches) label Oct 21, 2021
@josephperrott josephperrott removed the request for review from a team October 21, 2021 16:20
mmalerba
mmalerba commented Oct 21, 2021
View reviewed changes
Copy link
Contributor Author

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

@devversion There were a few TODOs buried in there that I'm not really sure how to fix. I commented on them to call them out, let me know if you've ever run into similar issues and know how to fix them

src/material/schematics/ng-generate/mdc-migration/schema.ts Outdated Show resolved Hide resolved
src/material/schematics/ng-generate/mdc-migration/index.ts Outdated Show resolved Hide resolved
devversion
devversion approved these changes Oct 21, 2021
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

Still LGTM; just added some more optional comments.

I didn't realize before that there were more changes, than just a new CLI project

integration/mdc-migration-example/src/styles.scss Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/material/schematics/ng-generate/mdc-migration/schema.ts Outdated Show resolved Hide resolved
@mmalerba mmalerba force-pushed the mdc-migration branch 4 times, most recently from 212c1c3 to e0c9236 Compare October 27, 2021 21:59
devversion
devversion reviewed Oct 28, 2021
View reviewed changes
src/material/schematics/ng-generate/mdc-migration/index.ts
@@ -50,10 +51,9 @@ function getComponentsToMigrate(requested: string[]): Set<string> {

export default function (options: Schema): Rule {
const componentsToMigrate = getComponentsToMigrate(options.components);
const pathToMigrate = path.resolve(options.path ?? process.cwd());
Copy link
Member

Choose a reason for hiding this comment

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

I'm just providing some context here: We should start using workspace-relative paths from the beginning on so that all updates/file modifications can go through the virtual devkit file tree.

So, technically relying on path.resolve & cwd is not something we should do. Not needed for this PR, just bringing this 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.

ack, added a TODO

@mmalerba mmalerba merged commit 155e4ab into angular:mdc-migration Oct 28, 2021
@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 Nov 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@devversion devversion devversion approved these changes

@jelbourn jelbourn Awaiting requested review from jelbourn

Assignees

@devversion devversion

Labels
cla: yes PR author has agreed to Google's Contributor License Agreement target: feature This PR is targeted for a feature branch (outside of main and semver branches)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mmalerba @devversion