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

Refactor: Audit custom event payload for consistency #2925

Closed
jcfranco opened this issue Aug 27, 2021 · 4 comments
Closed

Refactor: Audit custom event payload for consistency #2925

jcfranco opened this issue Aug 27, 2021 · 4 comments
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. refactor Issues tied to code that needs to be significantly reworked.

Comments

@jcfranco
Copy link
Member

jcfranco commented Aug 27, 2021

Summary

We have inconsistencies when it comes to emitting custom events:

  1. Emitting with a value
  • with an object, e.g., this.calciteFoo.emit({ foo: "foo" })
  • with a value, e.g., this.calciteFoo.emit("foo")
  1. Typing the event – EventEmitter is not typed consistently
  • typed based on the types from 1, e.g., calciteFoo: EventEmitter<FooDataObject>;
  • untyped calciteFoo: EventEmitter

For 1, we should emit an object for future-proofing, regardless of how many props are included.
For 2, we should always provide a type on EventEmitter. void should be used if there is no event payload. We can even add an ESLint rule to catch this.

Additionally, our conventions should be updated with this information and to also review whether it makes sense to introduce a prop instead of being part of an event's payload (to avoid cases where a prop from the event's detail ends up being a prop in the future and therefore redundant).

cc @benelan @samMatenaer

Additional resources

#2854
#2858
#3163

@jcfranco jcfranco added refactor Issues tied to code that needs to be significantly reworked. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Aug 27, 2021
@jcfranco jcfranco added this to the Freezer milestone Aug 27, 2021
@benelan benelan self-assigned this Aug 27, 2021
@benelan benelan added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Aug 27, 2021
@benelan benelan removed their assignment Mar 23, 2022
@benelan benelan added 0 - new New issues that need assignment. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Mar 23, 2022
@jcfranco jcfranco self-assigned this Jun 1, 2022
@jcfranco jcfranco modified the milestones: Freezer, Sprint 05/23 - 06/03 Jun 1, 2022
@jcfranco jcfranco added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels Jun 1, 2022
@jcfranco jcfranco removed the 1 - assigned Issues that are assigned to a sprint and a team member. label Jul 27, 2022
@driskull
Copy link
Member

driskull commented Dec 13, 2022

Here are some events I found that could deprecate their event payload or clean it up to be consistent.

  • calciteComboboxFilterChange
  • calciteDatePickerRangeChange
  • calciteDropdownSelect
  • calciteRatingChange
  • calciteTabChange
  • calciteTreeSelect
  • calcitePaginationChange
  • calciteDatePickerChange
  • calciteDatePickerActiveDateChange
  • calciteDatePickerSelect
  • calciteRadioGroupChange
  • calciteTabsActivate
  • calciteListChange
  • calciteListOrderChange

Some of them may require introducing a public property in order to remove the event payload.

@geospatialem should we make this a list for us to divvy up the work?

@driskull
Copy link
Member

This one should be good now.

@driskull driskull added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Dec 15, 2022
@github-actions github-actions bot assigned geospatialem and unassigned jcfranco and driskull Dec 15, 2022
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Jan 16, 2023
@geospatialem
Copy link
Member

Verification

  • 1. Event values with an object
    • All events have objects, confirmed on master branch
    • Note: There are a few private events and methods that use values such as toggleExpand (action-bar, action-pad), openHandler (multiple components), etc. Maybe something we could consider refactoring post-1.0?
  • 2. Event typings
    • All events verified on master branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. refactor Issues tied to code that needs to be significantly reworked.
Projects
None yet
Development

No branches or pull requests

4 participants