-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
export class SkyAutofillDirective implements OnInit { | ||
|
||
/** | ||
* Prevents the browser's native autofill from displaying for an input element. |
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.
Since this property allows "on" or "off", I would suggest changing the wording of the property's description to something like "Enables or disables the browser's native autofill for an input element."
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.
Updated, but also added similar copy to match the web API's definition for autocomplete
: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete.
* @required | ||
*/ | ||
@Input() | ||
public set skyAutofill(value: string) { |
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 looks like the only valid values are on
or off
. Should this be a distinct type like we've done elsewhere?
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.
After discussing, we've decided to accepting all strings. I've updated the logic.
} | ||
|
||
private setAutocomplete(element: ElementRef, value: string): void { | ||
if (value === 'on') { |
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.
This would be better as a switch
statement. Also, what about the case where value
is undefined? Should it be the same as on
?
switch (value) {
case 'off':
break;
default:
break;
}
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.
Done.
this.setAutocomplete(this.elementRef, this._autofill); | ||
} | ||
|
||
private setAutocomplete(element: ElementRef, value: string): void { |
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.
Since elementRef
and autofill
are instance properties, and setAutocomplete()
is an instance method, I'd get rid of the parameters and refer to the properties internally to the method. Also, you should only refer to private properties with underscores within a get
ter or set
ter; I'd fix this by adding a get
ter for skyAutofill
and referencing it in methods.
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.
Done.
|
||
if (isChrome) { | ||
const name = element.nativeElement.getAttribute('name') || 'sky-input'; | ||
this.renderer.setAttribute(element.nativeElement, 'autocomplete', `new-${name}`); |
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.
Instead of duplicating the this.renderer.setAttribute(element.nativeElement, 'autocomplete', x);
line, I'd suggest assigning a variable for the attribute value, then calling this.renderer.setAttribute()
outside the if
statement.
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.
Done.
export class SkyBrowserDetector { | ||
|
||
public static isChromeDesktop = ( | ||
navigator.userAgent.indexOf('Chrome') > -1 && |
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.
🤮 (though I understand it's necessary)
this.renderer.setAttribute(this.elementRef.nativeElement, 'autocomplete', value); | ||
break; | ||
|
||
case undefined || '': |
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.
Is the intent here to fall into this block when this.skyAutofill
is either undefined
or empty string? The way this is written, it's only going to fall in when it's an empty string, because undefined || ''
gets evaluated as ''
. If you want both values to fall into the same block, you need to specify two case
statements:
case undefined:
case '':
// Some logic
break;
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.
Oops. Forgot switch/case 101 there. Thanks for catching that. Fixed.
|
||
<div | ||
class="input-box-visual" | ||
class="input-box-visual sky-padding-even-large" |
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.
☝️ Prevents the browser's scroll bar from sneaking into the visual tests sometimes.
Codecov Report
@@ Coverage Diff @@
## master #157 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 29 32 +3
Lines 952 889 -63
Branches 127 170 +43
=========================================
- Hits 952 889 -63
Continue to review full report at Codecov.
|
Preview build at https://host.nxt.blackbaud.com/skyux-forms?_sp.spa=pr-sky-auto-fill |
@Blackbaud-PaulCrowder The changes to the fluid grid in |
blackbaud/skyux-lookup#104