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

Add unsubscribe function to event listener #35

Merged
merged 5 commits into from
Mar 23, 2023

Conversation

Peter-Maguire
Copy link
Contributor

@Peter-Maguire Peter-Maguire commented Mar 20, 2023

No description provided.

@Peter-Maguire Peter-Maguire self-assigned this Mar 20, 2023
@Peter-Maguire Peter-Maguire changed the title Use .addEventListener and .removeEventListener instead of .on in space Add unsubscribe function to event listener Mar 22, 2023
@Peter-Maguire Peter-Maguire marked this pull request as ready for review March 22, 2023 14:34
@dpiatek
Copy link
Contributor

dpiatek commented Mar 22, 2023

@Peter-Maguire could rebase and remove the initial 2 commits here? (I'm assuming you mean to squash the linting issue when merging?)

Copy link
Contributor

@dpiatek dpiatek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is suitable to what we have now, although ideally, we'd be able to have an .off method that does not require the developer to keep a reference to the handler, or clean-up.

For the moment, because we want to rethink what kind of event emitter we would like to use, which will impact what can do, I'm happy to approve.

* fix incorrect time dayjs function in status indicator

* rename data to profileData to reflect changes in the space library

* fix typo
@Peter-Maguire Peter-Maguire merged commit f0467b4 into main Mar 23, 2023
@Peter-Maguire Peter-Maguire deleted the allow_unsubscribing_from_events branch March 23, 2023 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants