Enable load animations on reset-rows handler event - #278
Conversation
edouardmisset
left a comment
edouardmisset
left a comment
There was a problem hiding this comment.
Left a question that needs some thoughts :)
| * @returns {TableHandler} | ||
| */ | ||
| on(event: string, handler: (...args: any[]) => any): TableHandler { | ||
| on(event: HandlerEvent, handler: (...args: any[]) => any): TableHandler { |
There was a problem hiding this comment.
warning: this a public interface change (same with off below).
- It might cause compilation errors
- Mistakes in the consuming packages
I would advice to release a new major version if you're confident and have tested/modified consuming packages
OR
Widen the type like LooseAutoComplete<HandlerEvent>
Food for thoughts.
There was a problem hiding this comment.
A little more context:
handler.ts:181: on() previously accepted any string event name, but now only accepts the closed HandlerEvent union. Meanwhile, triggerEvent() still accepts arbitrary strings at handler.ts:169, and the package emits the custom filterUpdateIntent event from numeric.ts:77. Existing TypeScript consumers subscribing to custom or currently omitted events will stop compiling even though runtime behavior is unchanged. Preserve the public string API for on() and off(), or add an explicitly supported extensibility overload while keeping the narrower type for replayOn.
There was a problem hiding this comment.
A quick github search showed me that the consumers used values from this array though but - am concerned by this too ! Kinda waiting on @phndiaye 's feedback on this :)
There was a problem hiding this comment.
Phil checked this one for me, it was causing issue, we went with the loose autocomplete route 🙏
edouardmisset
left a comment
edouardmisset
left a comment
There was a problem hiding this comment.
Left a question that needs some thoughts :)
upfluence-bot
added
reviewed/needs-work
and removed
reviewed/needs-review
reviewed/needs-work
labels
upfluence-bot
added
reviewed/needs-work
reviewed/needs-review
and removed
reviewed/needs-work
reviewed/needs-review
labels
afd88f2 to
7a72da2
Compare
upfluence-bot
added
reviewed/needs-work
and removed
reviewed/needs-review
reviewed/needs-work
labels
5 participants
What does this PR do?
Enable load animations on reset-rows handler event
Adds a new
replayOnparameter for theinitialLoadAnimationtable configuration, in which we can specify on whichHandlerEventwe want animations to be triggered. Currently it only handles thereset-rowsevent, but the code has been written to easily accept other events in the future, if required.Also adds an
offmethod to the Handler so that we can properly unsubscribe listeners.Good PR checklist