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

update(ripple): remove deprecated speed factor option #12258

Merged
merged 2 commits into from
Sep 21, 2018
Merged

update(ripple): remove deprecated speed factor option #12258

merged 2 commits into from
Sep 21, 2018

Conversation

devversion
Copy link
Member

@devversion devversion commented Jul 18, 2018 •
edited
Loading

  • Removes the deprecated [matRippleSpeedFactor] and global baseSpeedFactor option.
  • Adds update rules that can automatically switch to the new API (as much as possible) and even calculate the new durations based on the previously specified speed factor.

BREAKING CHANGE: deprecated [matRippleSpeedFactor] and baseSpeedFactor for the ripples have been removed. Use the new animation config instead.

@devversion devversion added the target: major This PR is targeted for the next major release label Jul 18, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 18, 2018
@devversion
Copy link
Member Author

@jelbourn I know the update tool needs to be updated to work for v6.x.x to v7.0.0, but in the meanwhile we can still have those rules in the repository.

I wasn't sure whether an auto-migration would be desired here, but it was pretty fun to do, so I'm fine removing them if they are not needed. Can also submit in a separate PR to keep this one smaller.

@devversion devversion force-pushed the v7/remove-ripple-speed-factor branch 2 times, most recently from e47473a to 336814a Compare July 18, 2018 13:18
@devversion devversion changed the title update(ripple): remove deprecated speed factor option [breaking change] update(ripple): remove deprecated speed factor option Jul 18, 2018
@devversion devversion force-pushed the v7/remove-ripple-speed-factor branch from 336814a to a3976bb Compare July 18, 2018 18:12
jelbourn
jelbourn reviewed Jul 19, 2018
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.

Would you be interested in updating the schematics code to be ready for going from v6 to v7? Right now everything under update/ is specifically for v5 -> v6, and it could probably be organized better

src/lib/schematics/update/rules/ripple-v7/switchRippleSpeedFactorRule.ts Outdated
[propertyNameReplacement, rightExpressionReplacement]);
} else {
// In case the speed factor is dynamically calculated or passed to the assignment, we just
// print the failure and notify about the breaking change.
Copy link
Member

Choose a reason for hiding this comment

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

Could you just treat the entire expression as the "value"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean with that? Do you say we should just create the actual calculation instead of executing it?

r.speedFactor = mySpeedFactor

// --->

r.animation = {enterDuration: (450 / mySpeedFactor)};

Copy link
Member

Choose a reason for hiding this comment

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

That's pretty much what I was thinking

Copy link
Member Author

@devversion devversion Jul 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We can totally do that, but not sure how much that would help with upgrading. That's more like a workaround to just make the upgrade succeed.

My goal with the rules would be to generate code that can actually stay in long-term, and in that specific situation, people should switch their whole (not just the assignment) code-base from speed factor to the new animation config.

I'm fine going any way here. Don't feel strong about this approach either.

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 it's nicer to make the update and add a TODO rather than fail the update. The ideal scenario with ng update is that you run it and your app works exactly the same way as before.

Copy link
Member Author

@devversion devversion Jul 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That makes sense. I will make the change but cannot guarantee about the TODO because it's not clear how the actual assignment is placed in the file. If that won't work, I will either print a message, or just do an inline TODO.

e.g. r.animation = /** TODO: Remove speed factor */ {enterDuration: (450 / mySpeedFactor)};

@devversion devversion force-pushed the v7/remove-ripple-speed-factor branch 2 times, most recently from 9f602dd to 1def6cc Compare July 19, 2018 10:09
@devversion
Copy link
Member Author

@jelbourn Sure! I've been having a look on the update tool since yesterday and I'm already trying to clean it up a bit (style-wise for now; linting etc.)

We should chat more about this. I'm not sure whether we want to keep the old migration data for example?

@devversion
Copy link
Member Author

devversion commented Jul 22, 2018 •
edited
Loading

@jelbourn Done. Thanks for the good idea for treating non-analyzable expressions.

Now, I think the ripple upgrade is fully covered and it's pretty advanced for something that isn't really that commonly used 😄. Please have another look

@devversion devversion force-pushed the v7/remove-ripple-speed-factor branch from ce824db to dd5b5c1 Compare July 22, 2018 10:09
jelbourn
jelbourn approved these changes Jul 24, 2018
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 pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jul 24, 2018
@devversion devversion force-pushed the v7/remove-ripple-speed-factor branch from dd5b5c1 to 9c7986b Compare July 30, 2018 22:03
@devversion devversion added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Aug 3, 2018
@ngbot
Copy link

ngbot bot commented Aug 7, 2018

Hi @devversion! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@devversion
update(ripple): remove deprecated speed factor option.
07992e3 
* Removes the deprecated `[matRippleSpeedFactor]` and global `baseSpeedFactor` option.
* Adds update rules that can automatically switch to the new API (as much as possible) and even calculate the new durations based on the previously specified speed factor.

BREAKING CHANGE: deprecated `[matRippleSpeedFactor]` and `baseSpeedFactor` for the ripples have been removed. Use the new animation config instead.
@devversion
Add test case; fix NodeJS version target circular dependency; Support…
1d8e392 
… for rule version constraints
@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Sep 19, 2018
@mmalerba mmalerba merged commit a6c91bc into angular:master Sep 21, 2018
roboshoes pushed a commit to roboshoes/material2 that referenced this pull request Oct 23, 2018
@devversion @roboshoes
update(ripple): remove deprecated speed factor option (angular#12258)
391e9b1 
* update(ripple): remove deprecated speed factor option.

* Removes the deprecated `[matRippleSpeedFactor]` and global `baseSpeedFactor` option.
* Adds update rules that can automatically switch to the new API (as much as possible) and even calculate the new durations based on the previously specified speed factor.

BREAKING CHANGE: deprecated `[matRippleSpeedFactor]` and `baseSpeedFactor` for the ripples have been removed. Use the new animation config instead.

* Add test case; fix NodeJS version target circular dependency; Support for rule version constraints
@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 Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@amcdnl amcdnl Awaiting requested review from amcdnl

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

@tinayuangao tinayuangao Awaiting requested review from tinayuangao

Assignees

@jelbourn jelbourn

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@devversion @jelbourn @googlebot @mmalerba