-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add pattern and autocomplete properties #601
Conversation
Codecov Report
@@ Coverage Diff @@
## master #601 +/- ##
==========================================
+ Coverage 98.94% 98.95% +<.01%
==========================================
Files 42 42
Lines 3043 3053 +10
Branches 820 823 +3
==========================================
+ Hits 3011 3021 +10
Misses 32 32
Continue to review full report at Codecov.
|
src/text-input/index.ts
Outdated
@@ -52,7 +54,7 @@ export const ThemedBase = ThemedMixin(WidgetBase); | |||
'labelAfter', | |||
'labelHidden' | |||
], | |||
attributes: [ 'widgetId', 'label', 'placeholder', 'controls', 'type', 'minLength', 'maxLength', 'value', 'name' ], | |||
attributes: [ 'widgetId', 'label', 'placeholder', 'controls', 'type', 'minLength', 'maxLength', 'value', 'name', 'pattern', 'autocomplete' ], |
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.
pattern will have to be a property if it's not just a 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.
I talked with @matt-gadd today and that doesn't seem to be the case
src/text-input/index.ts
Outdated
@@ -33,6 +33,8 @@ export interface TextInputProperties extends ThemedProperties, InputProperties, | |||
placeholder?: string; | |||
value?: string; | |||
shouldFocus?: boolean; | |||
pattern?: string | RegExp; | |||
autocomplete?: 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.
we should make this a boolean
and then reflect the right value depending on true
or false (which would mean it needs to be in properties
list for the custom element def too.
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.
autocomplete
can be more than "on"
and "off"
: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete
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.
Urgh
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.
boolean | string
?
@@ -33,6 +33,8 @@ export interface TextInputProperties extends ThemedProperties, InputProperties, | |||
placeholder?: string; | |||
value?: string; | |||
shouldFocus?: boolean; | |||
pattern?: string | RegExp; |
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.
Do you think we should a regular expression on the interface?
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 do because it's easier to catch RegExp errors in editors using RegExp syntax than in strings
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 I guess, but then it has to be a property for the custom elements, but it would be awesome if it was just an attribute. If you keep RegExp
on the interface, you are going to have to implement a custom diff. Because the instance's expression can be changed but it would not trigger a re-render.
@bryanforbes uh oh @smhigley has conflicted you! 💃 |
f73ce49
to
52906c9
Compare
Type: feature
The following has been addressed in the PR:
Description:
Adds
pattern
andautocomplete
properties and passes them through to the<input>
nodeResolves #553