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

[RFC] Browserlike Events #345

Open
2 tasks done
ZempTime opened this issue May 21, 2020 · 6 comments
Open
2 tasks done

[RFC] Browserlike Events #345

ZempTime opened this issue May 21, 2020 · 6 comments
Labels
enhancement New feature or request

Comments

@ZempTime
Copy link
Contributor

ZempTime commented May 21, 2020

Problem
I'm wondering if it would behoove us to:

  • Stick with existing browser-event names where possible
  • When and where it becomes prudent to introduce a new event, we maintain a browser-like like (ex: tabselect).

What's a browserlike event?
A short, all-lowercase, no delimiter imperative phrase like "click" or "mousemove".
If we need to use a delimiter, we could use - : . etc

Rollout:
This would involve two phases:

  • Minor version bump: we expose new events, support old events with a deprecation notice when consumed (so chameleon.tab-selected turns into tabselect, if we decide that would be better than click). Events continue to work as-is.
  • Major version bump after that: we remove support for non-browserspaced-events.

Questions & Research:

  • Is it possible to detect when an event is consumed by a listener and only display deprecation notices in those places?
  • Audit other existing component libraries and understand if there are any existing cowpaths here.
@ZempTime ZempTime added the enhancement New feature or request label May 21, 2020
@ZempTime
Copy link
Contributor Author

ZempTime commented Jun 17, 2020

I feel as though we should provide some guidance how/where to specify and use events.

Event Guide

In General:

  • When and where possible, set composed: false. This prevents extra computation, keeps clean boundaries between various document subtrees.
  • Events are intentionally a flexible and customizable abstraction whose value is the ability to work in whatever ways required. While this guide is used to instill some taste and consistency, we can't and shouldn't try to foresee all scenarios. Use your own judgement and feel free to break any of these if it makes sense.
  • When and where possible, rely on listeners using e.target to get the info they need. Adding extra info to the event might seem convenient at first, but it expands the api surface of the component. If you need to add extra information to the event, do so sparingly.

Should I prefix with chameleon?
Initially, we thought this would be a great way to delineate where events were coming from (bootstrap does this). However, it doesn't end up delivering any extra convenience. If you're pulling in a component for use, you know what it is. And if you're trying to figure out where an event came from, event.target has you covered. Let's save some keystrokes and drop the prefixes.

What if my event is like a browser event?
Examples here are input, blur, change, etc. If your custom element is dispatching an event to replicate existing browser events, use the same name. In general, events like this are the exception to the "don't use composed" guidance.

Is my event letting others places know something happened?
AKA, does your element have local state? If so, you're going to want to use a ${property}-${changed}-style format.

[rfc note: if a component can have local state, then they're not strictly unidirectional, and we shouldn't advertise this component library that way. Maybe, "conveniently" unidirectional]

For example, it makes complete sense for chameleon-accordion to maintain whether or not it's expanded. This means, when the expanded property changes, an expanded-changed event is appropriate. Note, we can reuse the tagname convention here: the presence of a - in an event name implies it's a custom element's event. This is clear, and conveys a lot of information in a small string. We can also assume that if someone is interacting with this event, they know what component it's relevant to. Therefore, keeping the property name in the event communicates useful information to the developer.

Is my event a part of the imperative interface of my component?
Phrased slightly differently, what if I want to respond to events by calling methods that modify component state?

For example, the sheet listens for a close-sheet event which, you guessed it, closes the sheet. Here, you're going to want to name the event imperatively (current- or future-tensed, as though it were a command) and make sure to include a -. Generally, if a simple phrase describes the action your component should take when this event is received, name it that.

Should I send more than one kind of event for different states?
If the listener can interrogate the event target to find that information (imagine an accordion-toggled event), no.

If it really simplifies or streamlines implementation, yes.

@ryuhhnn
Copy link
Contributor

ryuhhnn commented Jun 17, 2020

I agree 100% with dropping prefixes, especially given that most of the events that we are capturing live within the same component as the import, so you’re right, it should be pretty clear where that event came from (not to mention, <chameleon-input @chameleon.input.input> feels a little redundant 😛

Something worth noting here is that a lot of our form elements, we actually can just rely on their underlying native events (this might require some research follow up to see if the native events are composed or not). On that note too, I do think we should be very explicit when and when not to use composed events. Do you know of any prior art/literature on that?

I also think there should be more clear guidance on imperative events and the verbiage we use around it. I prefer past tense actions like sheet-closed given that technically we are dispatching these events after they happen. I’m not sure how we would specify which events are “imperative” though.

@ZempTime
Copy link
Contributor Author

ZempTime commented Jun 17, 2020

Re: existing cowpaths. No, I'm unaware of any.

In the case of close-sheet, it's a way of exposing the interface of the component (the method that closes the sheet) to components down the dom tree, as they won't have the ability to get or maintain a reference to sheet to call the toggle() method directly. This is important because since it can accept arbitrary slotted content, it could get arbitrarily deep. This let's us expose the ability for content inside the sheet to close it (as we do with the close icon in sheet-content). This inversion of control is important and grants a lot of flexibility.

So this is a case where we're actively, trying to expose a method in an imperative fashion, and our way of doing that is through an event (it's the reverse of passing properties down). When we want an element to modify it's internal state in response to it's descendants, I think it makes sense to dispatch an imperatively-named event which says "do this."

I think relying on underlying native elements is the right way to go, although these will themselves contain uncontrolled state.

@ryuhhnn
Copy link
Contributor

ryuhhnn commented Jun 17, 2020

Given everything that’s been mentioned here, I’m going to add a draft write up to the wiki (mostly just going to copy what you’ve written here) and we can ratify it once we feel that everyone has had a sufficient time to provide their feedback.

@ZempTime
Copy link
Contributor Author

ZempTime commented Jun 18, 2020

Here's a brief runthrough of current events, and their future state (if any obvious ones according to these guidelines). I think we're going to need to apply some thought to some of these.

place old new
packages/accordions/src/ChameleonAccordion.js#68: chameleon.accordions.expanded-changed expanded-changed
packages/tabs/src/ChameleonTab.js#19: chameleon.tabs.selected-changed selected-changed
packages/timezone/src/ChameleonTimezone.js#148: chameleon.timezone.input change, input
packages/input/src/ChameleonInput.js#285 chameleon.input.input change, input
packages/chip/src/ChameleonChip.js#33: remove-chip remove-chip
packages/toast/src/ChameleonToast.js#55 close-toast close-toast
packages/dialog/src/ChameleonDialog.js#92: toggle-dialog close-overlay
packages/dialog/src/ChameleonDialog.js#100 go-back
packages/date/src/ChameleonDate.js#366 chameleon.date.input input
packages/table/src/ChameleonTable.js#243: chameleon.table.change change
packages/select/src/ChameleonSelect.js#341 chameleon.select.input input
packages/modal/src/ChameleonModal.js#69 toggle-modal close-overlay
packages/paginator/src/ChameleonPaginator.js#182 page-change current-page-changed
packages/multiselect/src/ChameleonMultiselect.js#347: chameleon.select change, input
packages/multiselect/src/ChameleonMultiselect.js#360 chameleon.search search

@ZempTime
Copy link
Contributor Author

ZempTime commented Jun 18, 2020

As a point of clarity, close-overlay is what closes the sheet. It doesn't close, and then dispatch that event. The sheet listens for and responds to the event by closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants