-
Notifications
You must be signed in to change notification settings - Fork 58
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
CPLAT-12547 Update SyntheticEvent Usage #639
CPLAT-12547 Update SyntheticEvent Usage #639
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
@@ -793,7 +742,7 @@ main() { | |||
}); | |||
|
|||
expect( | |||
logs.first, | |||
logs[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.
Because of the Cannot find native JavaScript type (SyntheticMouseEvent) for type check
warning (caused by the removal of the @anonymous
annotations from SyntheticEvent
js interop classes in react-dart
), the expected warning appears second in the logs.
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.
Also, this is the only non-formatting change in this file.
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.
Updating the test in this way makes it depend on that error existing, but that won't always be the case. Can we instead update it to ignore that error, so that it won't start failing again if that error goes away?
final logs = await recordConsoleLogsAsync(() async {
click(fluxButton);
await Future(() {});
});
logs.removeWhere((log) => log.contains('Cannot find native JavaScript type');
expect(
logs.first,
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.
Ohh yep, good call. I added that!
…17-synthetic-event-updates
@@ -1,161 +0,0 @@ | |||
// Copyright 2016 Workiva Inc. |
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.
Removed this because it got moved to react-dart
.
/// Helper util that wraps a native [KeyboardEvent] in a [SyntheticKeyboardEvent]. | ||
/// | ||
/// Used where a native [KeyboardEvent] is given and a [SyntheticKeyboardEvent] is needed. | ||
SyntheticKeyboardEvent wrapNativeKeyboardEvent(KeyboardEvent nativeKeyboardEvent) { |
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 was removed because it got moved to react-dart
.
+1 (on Joe's changes) |
@@ -793,7 +742,7 @@ main() { | |||
}); | |||
|
|||
expect( | |||
logs.first, | |||
logs[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.
Updating the test in this way makes it depend on that error existing, but that won't always be the case. Can we instead update it to ignore that error, so that it won't start failing again if that error goes away?
final logs = await recordConsoleLogsAsync(() async {
click(fluxButton);
await Future(() {});
});
logs.removeWhere((log) => log.contains('Cannot find native JavaScript type');
expect(
logs.first,
lib/src/util/event_helpers.dart
Outdated
); | ||
} | ||
export 'package:react/react.dart' | ||
show wrapNativeKeyboardEvent, wrapNativeMouseEvent, fakeSyntheticFormEvent, SyntheticEventTypeHelpers, DataTransferHelper; |
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.
#nit SyntheticEventTypeHelpers an DataTransferHelper are being exported unnecessarily
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.
Ahh yep, good catch. I realized that all of that really should have just been exported with the event stuff from react-dart anyway, so I just move all the exports into over_react.dart
. Let me know if I missed something though and it was important to keep this file!
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.
+10
Motivation
React-dart 6.0.0 makes changes to
SyntheticEvent
s that OverReact needed to respond to.Changes
Release Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: