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

Standardize widget events #302

Closed
smhigley opened this issue Sep 12, 2017 · 9 comments
Closed

Standardize widget events #302

smhigley opened this issue Sep 12, 2017 · 9 comments

Comments

@smhigley
Copy link
Contributor

Enhancement

The approach to widget events is fragmented across existing widgets. Basic form widgets have handlers for all interaction events (mouse, touch, and keyboard), and pass back the dom event object as the argument. Most other widgets only include widget-specific events (e.g. onOpen for Dialog), and pass some part of requested state back as the argument (e.g. year for onYearChange in Calendar).

Widgets could be improved by standardizing event handlers in two ways:

  • More logical approach to which events are exposed
  • Consistent approach to which arguments are passed to the event handler
@smhigley
Copy link
Contributor Author

My approach would be as follows:

Event handler arguments

  • Stop passing through the event object in all cases
  • In addition to passing through whatever values make sense for the specific event (i.e. value for onChange or open for onMenuChange), we should always pass back the widget key

We've already run into cases where we need to know which of an array of VNodes triggered an event. Right now we've gotten around it by implementing an index property in those specific widgets, but we should have a more general solution that works for end users as well. Key seems perfect, especially if we suggest keys be unique within a given widget rather than just among siblings.

Which Events?
It makes sense that an end user would want the full set of ui events on a form field but not on a more complex widget, but a more formal rule might be:

Widgets with a single interactive node support all user interaction events, whereas more complex widgets expose event handlers when a state change occurs or is requested

In context of the current widgets, this would mean ComboBox, TimePicker, Select, and SplitPane should support the full set of onClick, onTouchStart, etc. along with the other form widgets. All other widgets should be fine with the events they currently have.

@smhigley
Copy link
Contributor Author

From the comment on #266:

Update widget events to not return the event object. Instead, we should return whatever value is appropriate to the function (e.g. index for onRequestTabChange in TabController), along with the optional widget key for all event handlers. Always passing back the widget key allows users to have a single handler for multiple widgets with individual widget logic. For example, a form widget with multiple form fields could have a single onInput handler for all of them:

_input1 = '';
_input2 = '';
_input3 = '';

private _onInput(value: string, key: string | number) {
  this[`_${key}`] = value;
  this.invalidate();
}

render() {
  return v('div', [
    w(TextInput, {
      key: 'input1',
      onInput: this._onInput
    }),
    w(TextInput, {
      key: 'input2',
      onInput: this._onInput
    }),
    w(TextInput, {
      key: 'input3',
      onInput: this._onInput
    })
  ]);
}

@smhigley
Copy link
Contributor Author

The other option is to use a separate common identifier like index instead of key, so long as it's standardized across all event handlers in all widgets.

@pottedmeat
Copy link
Contributor

By not returning the object, are we thinking that we'd relay meaningful values from the underlying node (like .value) through the new event object?

@smhigley
Copy link
Contributor Author

Yup, for events like onchange and oninput, I think it would make sense to have something like onChange?(value: string; key?: string | number). Keyboard events should probably have either event.which or event.key. I'm not sure if mouse, touch, and focus events need anything beyond the key/identifier passed back.

This does remove the ability to call preventDefault. I'm not entirely sure how much that will be an issue :P

@pottedmeat
Copy link
Contributor

I think passing that through on events where it matters is a good idea

@bitpshr
Copy link
Member

bitpshr commented Oct 20, 2017

Would we create an Event object and selectively add information to it, which would allow functionality like preventDefault to be preserved, or would we pass raw values to property callbacks?

@pottedmeat
Copy link
Contributor

What do you mean by raw values? I think we'd like to create an event object and selectively add info to it.

@tomdye
Copy link
Member

tomdye commented Oct 23, 2017

I agree that we should change and standardise the params that we pass to callbacks from our vdom events.

This will in part be achieved by the changes to widget-core to inline and utilise our own vdom implementation as this will allow us to abstract the dom event away and pass our own event including the vdom key.

In terms of stopPropogation etc... I believe that widget-core will control the event bubbling and will provide a means to prevent it as well as stopping events from bubbling outside of widget boundaries by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants