-
Notifications
You must be signed in to change notification settings - Fork 25
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: add control event to all state references with change event #388
Conversation
Codecov Report
@@ Coverage Diff @@
## tagged-release/q1-release #388 +/- ##
=============================================================
+ Coverage 93.64% 93.66% +0.01%
=============================================================
Files 43 43
Lines 1636 1641 +5
Branches 352 353 +1
=============================================================
+ Hits 1532 1537 +5
Misses 102 102
Partials 2 2
Continue to review full report at Codecov.
|
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.
Nice! Just a few minor ones.
Primitive.CheckboxField, | ||
Primitive.PasswordField, | ||
Primitive.PhoneNumberField, | ||
Primitive.Radio, |
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 we need the underlying radio as well?
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.
No we don't need it, bad copy paste.
'FullForm', | ||
'Listings', | ||
'ListingComp', | ||
'RoudedImage', |
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.
Probably don't need/want to commit this.
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.
Messed up the rebase. Will fix
2c3a365
to
d3e5ebf
Compare
[stateReference.property]: propertyReferences.concat([ | ||
{ addControlEvent: PrimitivesWithChangeEvent.has(componentType as Primitive) }, | ||
]), |
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.
Wouldn't we want the addControlEvent flag here to effectively be either the old logic or this new logic? Is there a usecase where we want to add a control event to a non-form element? Could be not, but it may be safer in the short term to leave the old behavior in until we convince our selves otherwise.
d3e5ebf
to
abcff11
Compare
No description provided.