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

refactor: move away from deprecated apis #3836

Merged
merged 1 commit into from
Apr 28, 2017
Merged

refactor: move away from deprecated apis #3836

merged 1 commit into from
Apr 28, 2017

Conversation

crisbeto
Copy link
Member

Moves away from the APIs that were deprecated in Angular 4. They include:

  • Switching from using OpaqueToken to InjectionToken.
  • Moving from Renderer to Renderer2.
  • Changing the Injector.get(thing: any) usages to use the new signature Injector.get<T>(thing: T).

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 29, 2017
@crisbeto crisbeto force-pushed the ng-4-deprecated-apis branch 2 times, most recently from 1ef8835 to 7579ed8 Compare March 29, 2017 21:38
jelbourn
jelbourn reviewed Apr 5, 2017
View reviewed changes
src/lib/button-toggle/button-toggle.spec.ts
@@ -5,7 +5,7 @@ import {
ComponentFixture,
TestBed,
} from '@angular/core/testing';
import {NgControl, FormsModule, ReactiveFormsModule, FormControl} from '@angular/forms';
import {NgModel, FormsModule, ReactiveFormsModule, FormControl} from '@angular/forms';
Copy link
Member

Choose a reason for hiding this comment

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

@kara is NgModel the right type here?

src/lib/button/button.ts
@@ -141,13 +141,17 @@ export class MdButton implements OnDestroy {

_setElementColor(color: string, isAdd: boolean) {
if (color != null && color != '') {
this._renderer.setElementClass(this._getHostElement(), `mat-${color}`, isAdd);
if (isAdd) {
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 you can kill this entire function now and just do the null check in _updateColor and call addClass / removeClass directly there.

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is used in a couple of other places, as well as inside the button.html so it may be more convenient to keep it. Otherwise we'd need to do this._elementRef.nativeElement all over the place.

src/lib/core/ripple/ripple.ts Outdated
export type RippleGlobalOptions = {
disabled?: boolean;
baseSpeedFactor?: number;
};

/** Injection token that can be used to specify the global ripple options. */
export const MD_RIPPLE_GLOBAL_OPTIONS = new InjectionToken<RippleGlobalOptions>(
Copy link
Member

Choose a reason for hiding this comment

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

Generally prefer line breaking at the higher syntactic level, e.g.:

export const MD_RIPPLE_GLOBAL_OPTIONS = 
    new InjectionToken<RippleGlobalOptions>('md-ripple-global-options');

(at the assignment)

@crisbeto
Copy link
Member Author

crisbeto commented Apr 5, 2017

Rebased and addressed some of the feedback @jelbourn.

jelbourn
jelbourn approved these changes Apr 6, 2017
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 and removed pr: needs review labels Apr 6, 2017
@jelbourn
Copy link
Member

jelbourn commented Apr 11, 2017 •
edited
Loading

This is a breaking change because Renderer was part of the API for focus origin monitor.

Caretaker will need to update Google call sites.

@jelbourn jelbourn added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Apr 11, 2017
@kara kara added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Apr 21, 2017
@kara kara assigned crisbeto and unassigned jelbourn Apr 21, 2017
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Apr 22, 2017
@crisbeto
Copy link
Member Author

Rebased. Should I remove the presubmit failures tag @kara?

@crisbeto crisbeto force-pushed the ng-4-deprecated-apis branch 2 times, most recently from afff9f5 to dd3329e Compare April 28, 2017 16:50
@crisbeto
refactor: move away from deprecated apis
1c03ac6 
Moves away from the APIs that were deprecated in Angular 4. They include:
* Switching from using `OpaqueToken` to `InjectionToken`.
* Moving from `Renderer` to `Renderer2`.
* Changing the `Injector.get(thing: any)` usages to use the new signature `Injector.get<T>(thing: T)`.
@mmalerba
Copy link
Contributor

It looks like the presubmit failures are because one of the teams in google is injecting the FocusOriginMonitor and the Renderer for one of their components and then calling focusVia with the Renderer. Do we want to make the FocusOriginMonitor accept Renderer2 | Renderer? or should we just refactor that project's code?

@crisbeto
Copy link
Member Author

crisbeto commented Apr 28, 2017 •
edited
Loading

It may be better to refactor it. We could have a solution that accepts both, but it would add some bloat and at some point we'll have to deprecate it anyway.

@jelbourn
Copy link
Member

@mmalerba if it's not widespread we should just change the team's code

@mmalerba mmalerba removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Apr 28, 2017
@mmalerba mmalerba merged commit 29e503a into angular:master Apr 28, 2017
@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 6, 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

Assignees

@crisbeto crisbeto

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

Successfully merging this pull request may close these issues.
5 participants
@crisbeto @jelbourn @mmalerba @kara @googlebot