Skip to content
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

#3347: include trigger event data #3348

Merged
merged 3 commits into from
May 8, 2022
Merged

Conversation

twschiller
Copy link
Contributor

@twschiller twschiller commented May 7, 2022

What does this PR do?

Out of Scope

  • Include an official reader with an output schema for event like we do for context menus. There's not much advantage since the shape changes per event
  • Show in the the preview of the data panel. (Also would require creating a fake reader)

@twschiller twschiller requested a review from fregante May 7, 2022 00:06
@twschiller twschiller added this to the 1.6.2 milestone May 7, 2022
@twschiller twschiller self-assigned this May 7, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 7, 2022

Codecov Report

Merging #3348 (3dd1b39) into main (74b411e) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #3348      +/-   ##
==========================================
- Coverage   36.86%   36.85%   -0.02%     
==========================================
  Files         751      751              
  Lines       21520    21527       +7     
  Branches     4600     4602       +2     
==========================================
  Hits         7934     7934              
- Misses      12669    12675       +6     
- Partials      917      918       +1     
Impacted Files Coverage Δ
src/extensionPoints/triggerExtension.ts 8.94% <0.00%> (-0.27%) ⬇️
...c/pageEditor/tabs/trigger/TriggerConfiguration.tsx 32.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74b411e...3dd1b39. Read the comment docs.

@@ -196,6 +216,7 @@ export abstract class TriggerExtensionPoint extends ExtensionPoint<TriggerConfig
*/
// Can't set in constructor because the constructor doesn't have access to debounceOptions
private debouncedRunTriggersAndNotify?: (
nativeEvent: Event | null,
...roots: ReaderRoot[]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you turn this into a non-spread array you can have it as the first parameter. This way you don't have to pass around NO_NATIVE_EVENT since it's the second parameter and can be empty. It's slightly less convenient but IMHO better than an optional first parameter.

- await this.debouncedRunTriggersAndNotify(NO_NATIVE_EVENT, ...$root);
+ await this.debouncedRunTriggersAndNotify($root.get()); // jQuery -> Array
- void this.debouncedRunTriggersAndNotify(NO_NATIVE_EVENT, element);
+ void this.debouncedRunTriggersAndNotify([element]); // Single element -> Array

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A swapped the order and made an object for nullable params. I kept the param as non-optional because I want to make the behavior very explicit for readability

@@ -155,6 +159,22 @@ async function interval({
console.debug("interval:completed");
}

function pickEventProperties(nativeEvent: Event): UnknownObject {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be typed:

Suggested change
function pickEventProperties(nativeEvent: Event): UnknownObject {
function pickEventProperties(nativeEvent: Event): SerializableKeyboardEvent | null {
interface SerializableKeyboardEvent {
	key: number;
	keyCode: number;
	metaKey: boolean;
	altKey: boolean;
	shiftKey: boolean;
	ctrlKey: boolean;
}

Copy link
Contributor Author

@twschiller twschiller May 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be typed

I don't think there's much value in typing it yet because the caller doesn't care what it gets back as long as it's serializable. The return type should be JsonObject. But picks type signature isn't precise enough to support that

@twschiller twschiller merged commit 0ebb4d2 into main May 8, 2022
@twschiller twschiller deleted the feature/3347-trigger-event-data branch May 8, 2022 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass native keyboard event data with trigger
3 participants