EnsembleUI / ensemble

Build native apps 20x faster than Flutter, RN or any other tech
https://ensembleui.com/
BSD 3-Clause "New" or "Revised" License
125 stars 15 forks source link

Event callback for custom widget #204

Closed vusters closed 8 months ago

vusters commented 1 year ago
View:
  Column:
    children:
      - CustomWidget:
           inputs:
             label: My Special Widget
           events:
             onDateSelected: 
                 invokeAPI:
                    name: getReservations
                    inputs:
                        date: ${event.date} #this.date should work too I think

CustomWidget:
  inputs:
    - label
  events:
    - onDateSelected:
          data: {date, month, year}
  body:
    Button:
      onTap:
        dispatchEvent:
           onDateSelected:
                 data:
                   date: ${cal.getDate()}
                   month: ${cal.getMonth()}
                   year: ${cal.getYear()}
kmahmood74 commented 10 months ago

assigning to myself as Vu is busy. The Yaml is not right as it confuses inputs and callbacks. Those are two different concepts.

There are 3 concepts in Custom Widgets (or any widget for that matter) that need to be clearly defined and documented -

  1. Inputs - this is data or configuration that the widget needs to display/act correctly. Think of specific styling or the label of a field being passed as input

  2. Events - these are the events that the widget dispatches. For example - onDateSelected dispatched by a Calendar widget. UI components are asynchronous and UI is an event driven system, that's why widgets dispatch events to communicate changes. They don't have outputs i.e. return values that you expect from a synchronous method call

  3. Event Handlers - these are actions that the caller can attach to events. Ensemble actions are perfect for this.

Here's the proposed YAML. This will also work well in the schema and allow us to auto-complete event handlers

View:
  Column:
    children:
      - CustomWidget:
           inputs:
             label: My Special Widget
           events:
             onDateSelected: 
                 invokeAPI:
                    name: getReservations
                    inputs:
                        date: ${event.date} #this.date should work too I think

CustomWidget:
  inputs:
    - label
  events:
    - onDateSelected:
          data: {date, month, year}
  Button:
    onTap:
      dispatchEvent:
          onDateSelected:
               data:
                   date: ${cal.getDate()}
                   month: ${cal.getMonth()}
                   year: ${cal.getYear()}
kmahmood74 commented 10 months ago

@vusters @amin-nas @snehmehta @evshi please review so I may get started on implementing it. See my comment above for proposed yaml

evshi commented 10 months ago

User is essentially defining custom events, but EDL has replaced event nomenclature with actions. For consistency and better developer experience, I would suggest renaming some keys: eventHandlers -> actions, dispatchEvent -> dispatchAction, parameters -> inputs and make it a list of strings.

kmahmood74 commented 10 months ago

good suggestion, thanks @evshi will make the change

kmahmood74 commented 10 months ago

@vusters what do you think?

kmahmood74 commented 10 months ago

@evshi these are not actions, they are events. An action is something that is invoked when an event takes place. for example onTap is an event and invokeAPI is an action. It doesn't make sense to mix the two. You don't dispatch an action, you dispatch an event. Actually it will be more confusing to mix the two as people will be dispatching invokeAPI and other actions if we call it dispatchAction and mix the two concepts.

I am going to revert to calling them events. It is cleaner

amin-nas commented 10 months ago

The proposed yaml is confusing to me. I expect to pass in an action to a widget, and decide when to fire it inside the widget.

View:
  Column:
    children:
      - CustomWidget:
           inputs:
             label: My Special Widget
             myAction: 
                 invokeAPI:
                    name: getReservations
                    inputs:
                        date: ${event.date}

CustomWidget:
  inputs:
    - label
    - myAction
  Button:
    onTap:
        myAction:
             inputs:
                 date: ${cal.getDate()}

I can also see an alternative where you define event handlers when instantiating the widget, and dispatch those events inside the widget definition:

View:
  Column:
    children:
      - CustomWidget:
           inputs:
             label: My Special Widget
           events:
             onDateSelected: 
                 invokeAPI:
                    name: getReservations
                    inputs:
                        date: ${event.date}

CustomWidget:
  inputs:
    - label
  events:
    - onDateSelected
  Button:
    onTap:
        dispatchEvent:
             name: onDateSelected
             inputs:
                 date: ${cal.getDate()}
vusters commented 10 months ago

How about this?

View:
  body:
    MyWidget:
      inputs:
        name: Peter Parker
      eventHandlers:
        onMyTap: |-
          log(event.data.occupation);

MyWidget:
  inputs: [name]
  eventHandlers: [onMyTap]  # encourage prefix on* for consistency
  body:
    Button:
      onTap:
        dispatchEvent:
          handler: onMyTap    # 'name' is OK too for similar convention with others
          data:
            occupation: superhero
vusters commented 10 months ago

Alternatively to be more consistent:

View:
  body:
    MyWidget:
      inputs:
        name: Peter Parker
      actionHandlers:
        onMyTap:
          log (event.data.occupation);  

MyWidget:
  inputs: [name]
  actionHandlers: [onMyTap]  # encourage prefix on* for consistency
  body:
    Button:
      onTap:
        callActionHandler:
          handler: onMyTap    # 'name' is OK too for similar convention with others
          data:
            occupation: superhero
amin-nas commented 10 months ago

Alternatively to be more consistent:

View:
  body:
    MyWidget:
      inputs:
        name: Peter Parker
      actionHandlers:
        onMyTap:
          log (event.data.occupation);  

MyWidget:
  inputs: [name]
  actionHandlers: [onMyTap]  # encourage prefix on* for consistency
  body:
    Button:
      onTap:
        callActionHandler:
          handler: onMyTap    # 'name' is OK too for similar convention with others
          data:
            occupation: superhero

I like this but seems inconsistent in the following:

kmahmood74 commented 10 months ago

https://github.com/EnsembleUI/ensemble/issues/204#issuecomment-1874724924 Kindly read my comment linked above. Actions and Events are two different concepts that should not be confused. I know we are trying to reduce the concept overload but by doing that we are adding more confusion and making things less clear.

Every UI dev understands events like onTap, onChange etc. Let's call them what they are i.e. Events Actions, as the name suggests are commands to do something. In the UI world, they are generally tied to events as eventHandlers.

ActionHandler as a term may be redundant as Action itself is a verb.

Secondly, I debated quite a bit about calling it eventHandlers vs event. I decided to go with events because it will be consistent between how the widget is defining it and how the caller is using it.

Lastly, events need to specify their parameters right. You keep missing that in your suggestions. How would anyone write a handler if they don't know what the payload of an event is.

Anyway, I'll go with what I proposed. Happy to chat more if you like.

kmahmood74 commented 10 months ago

updating the schema after a design meeting with @vusters and @amin-nas thanks guys

kmahmood74 commented 10 months ago

as soon as the PR is approved and merged, I'll add kitchen sink example and documentation

kmahmood74 commented 10 months ago

I have implemented the events but not very happy with it. It will encourage writing code instead of using bindings.

I think we should have the outputs as well so people may bind to them. Bindings are event-driven and are a short-hand for 'onDataChange' events. We should make it easy for people to use bindings with CustomWidget outputs.

It's really getters and setters. Perhaps we should call them something better than inputs and outputs. May be just call them properties?

See example below -

View:
  Column:
    children:
      - Text:
           text: ${calendar.selectedDate}
      - CustomWidget:
           id: calendar
           inputs:
             label: My Special Widget

CustomWidget:
  inputs:
    - label
  outputs: {selectedDate, currentMonth, selectedDateRange}
  body:
    Button:
      onTap: |-
        selectedDate = '12/01/2024';
evshi commented 10 months ago

I vote for properties, for the reasons you outlined above. It fits better with the Ensemble model today, which is primarily Object Oriented and not Event Driven/Functional. Events/Functions are not first class constructs in the YAML today - they are implied but never explicitly defined.

Might be helpful to have a concrete use case to frame the discussion around.

kmahmood74 commented 10 months ago

I'll create a separate ticket for properties. Everything can be replicated with properties. What I am not sure about is whether it makes sense to just use properties in all cases.

Example - Take Button as case. Instead of a Button.onTap, one can put a property called tapped which is set to true/false. It will still work but it just is weird to do that. It also doesn't allow someone to

We can make the binding even with events such as -

Text:
   text: ${myCalendar.onDateSelected.selectedDate}

but it seems redundant. I think I'll create a separate ticket for properties. Eventually if we decide we can take one out of the schema and hide it.

kmahmood74 commented 10 months ago

also Ensemble itself is not consistent here. In some cases, most notable being APIs, we decide to go with event-driven framework and have onResponse, onError and so on when we could just do - response as null or not.

So events do allow you to process data before passing it on to a binding

kmahmood74 commented 10 months ago

I think finally I have an opinion on this. We need events, outputs (getters) and methods on Custom widgets

Events - to notify when something happens. Example - onTap: users tapped on the field outputs - expose internal state at any time (not just in response to an event) so it could be used by the callers. Example: field.value so I may send it to the server. This is not in response to an event. methods - this is how callers control the behavior of a widget. For example - Map (moveCamera, runAutoZoom etc),

Here's why -

  1. Every interactive widget has events. This is not just true for any UI component platform but very much part and parcel of how Ensemble works. Most of our widgets expose events. Examples - Button (onTap etc), TextInput (onDelayedKeyPress etc), Image (onTap etc), Toggle (onChange etc), Map (onMapCreated, onCameraMove etc) and so on.

  2. Every widget exposes its state through properties, we call them getters in our Ensemble code. This is pretty obvious - Text.text is a getter as an example.

  3. Most widgets do expose methods as well.

This ticket covers events for CustomWidgets and once the PR is approved, events would be completed. Here's a ticket to cover the rest - #1126

Let me know what you think.

kmahmood74 commented 10 months ago

this was completed a few days ago. Updated the schema, will create kitchen sink and documentation and paste the links here and only then close it.