-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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.
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
feat: nullability support #5094
Conversation
d1edd16
to
9d4c411
Compare
src/lib/slide-toggle/slide-toggle.ts
Outdated
@@ -318,7 +318,7 @@ class SlideToggleRenderer { | |||
|
|||
/** Resets the current drag and returns the new checked value. */ | |||
stopThumbDrag(): boolean { | |||
if (!this.dragging) { return; } | |||
if (!this.dragging) { return this.dragging; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note for your wip: This is can be always return false
7614fb6
to
62bd6f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a handful of minor comments
@@ -65,13 +65,13 @@ export class MdButtonToggleGroup implements AfterViewInit, ControlValueAccessor | |||
private _name: string = `md-button-toggle-group-${_uniqueIdCounter++}`; | |||
|
|||
/** Disables all toggles in the group. */ | |||
private _disabled: boolean = null; | |||
private _disabled: boolean | null = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not default this to false
?
(same for other boolean properties)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: Just saw your comment below. I'll refactor the bindings to disabled || null
.
It's because the disabled
attribute needs to be removed from the DOM completely when enabled. We could also switch the bindings to be something like disabled || null
.
if (month < 0 || month > 11 || date < 1) { | ||
return null; | ||
if (month < 0 || month > 11) { | ||
throw new Error(`Invalid month index "${month}". Month index has to be between 0 and 11.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can omit the new
for Error
(woo, 4 bytes)
} | ||
|
||
if (date < 1) { | ||
throw new Error(`Invalid date "${date}". Date has to be larger than 0.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"larger than" > "greater than"
@@ -94,7 +94,7 @@ export class ConnectedPositionStrategy implements PositionStrategy { | |||
* @param element Element to which to apply the CSS styles. | |||
* @returns Resolves when the styles have been applied. | |||
*/ | |||
apply(element: HTMLElement): Promise<void> { | |||
apply(element: HTMLElement): Promise<null> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we work around this by having a custom type declaration to overload resolve such that resolve(): Promise<void>
?
Promise<null>
=> (╯°□°)╯︵ ┻━┻
(alternatively, apply
is and always has been synchronous, so we could just take the promise away)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what that would look like, null | undefined
? Otherwise I'm in favor of removing the promise completely, although I could see it being useful if we decide to introduce animations into the overlays.
src/lib/core/portal/portal.ts
Outdated
viewContainerRef: ViewContainerRef = null, | ||
injector: Injector = null) { | ||
viewContainerRef?: ViewContainerRef, | ||
injector?: Injector) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could make these | null
just to be more lenient (anywhere with multiple optional params where you might want to pass a latter one but an earlier one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with optionals here, because they were marked as [Optional]
a little bit up in the doc strings for the properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I accidentally omitted the word "also". Meant that we could do
viewContainerRef?: ViewContainerRef | null,
so that either undefined
or null
can be passed in cases when you don't care for that argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Added.
src/lib/core/ripple/ripple.spec.ts
Outdated
let viewportRuler: ViewportRuler; | ||
|
||
/** Extracts the numeric value of a pixel size string like '123px'. */ | ||
const pxStringToFloat = (s: string | null) => s ? parseFloat(s.replace('px', '')) : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it was already like this, but doesn't parseFloat
already ignore trailing non-numeric characters already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. This can be changed to parseFloat(...) || 0
.
src/lib/dialog/dialog.spec.ts
Outdated
|
||
expect(() => dialogRef = dialog.open(DialogWithInjectedData)).not.toThrow(); | ||
expect(dialogRef.componentInstance.data).toBeNull(); | ||
// TODO: double check that this does what i expect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ops, forgot to follow up on this one. I wanted to check that the expect
inside the expect.not.toThrow
won't cause any issues. I'll check it and refactor or remove the todo.
src/lib/icon/icon-registry.ts
Outdated
* @docs-private | ||
*/ | ||
export function getMdIconFailedToSanitizeError(url: SafeResourceUrl): Error { | ||
return new Error(`MdIconRegistry has failed to sanitize the following URL: ${url}.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more accurate error message would be something like "The url provided to MdIconRegistry was not trusted as a resource URL via Angular's DomSanitizer"
src/lib/menu/menu-item.ts
Outdated
@@ -44,7 +44,7 @@ export class MdMenuItem implements Focusable { | |||
} | |||
|
|||
/** Used to set the HTML `disabled` attribute. Necessary for links to be disabled properly. */ | |||
_getDisabledAttr(): boolean { | |||
_getDisabledAttr(): true | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of doing disabled || null
in the host binding
This means that the attribute is correctly removed in the DOM but programmatic APIs return the expected true
/false
c1c7dcd
to
5e9b13e
Compare
Rebased and addressed the feedback @jelbourn. |
5e9b13e
to
5ebfed6
Compare
08da2d8
to
ced3171
Compare
ced3171
to
02e42fb
Compare
02e42fb
to
d6f8f5e
Compare
@crisbeto This one needs rebase too, when you get a chance. |
d6f8f5e
to
e6e96c1
Compare
e6e96c1
to
6b15671
Compare
I rebased it (was trivial) |
6b15671
to
48e822b
Compare
Adds compatibility with strictNullChecks to the library, tests, build and various test apps. Fixes angular#3486.
48e822b
to
890052c
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Adds compatibility with strictNullChecks to the library, tests, build and various test apps.
Fixes #3486.