Skip to content

Conversation

@joshua-dean
Copy link
Collaborator

@joshua-dean joshua-dean commented Oct 7, 2024

Pull main event listeners into their own file

Description

As mentioned here, ULabel has static methods that take a class instance, and calling those gets problematic. This PR breaks out event listeners and the night mode cookie.

  • Added listeners.ts
    • create_ulabel_listeners - called during ULabel.init
    • remove_ulabel_listeners - ULabel.remove_listeners calls this but is kept for convenience
    • Long handlers have their own functions now
  • Added cookies.ts
    • Static class NightModeCookie
  • Updated a ton of type declarations

index.js is 400 lines shorter which is a win. Passing around the whole ULabel object isn't ideal but this is a step in the right direction.

PR Checklist

  • Merged latest main
  • Version number in package.json has been bumped since last release
  • Version numbers match between package package.json and src/version.js
  • Ran npm install and npm run build AFTER bumping the version number
  • Updated documentation if necessary (currently just in api_spec.md)
  • Added changes to changelog.md

Breaking API Changes

If anyone was explicitly calling ULabel.create_listeners rather than letting init do it, that functionality is gone.

@joshua-dean joshua-dean self-assigned this Oct 7, 2024
@joshua-dean joshua-dean added the enhancement New feature or request label Oct 7, 2024
@joshua-dean
Copy link
Collaborator Author

@TrevorBurgoyne @csolbs24 let me know what you think.

This doesn't change any functionality so I'm not that hurried to get this merged. If I can get a couple other organizational PRs together perhaps it would be nicer to bundle them.

This was referenced Oct 7, 2024
@joshua-dean
Copy link
Collaborator Author

Marking this as "ready for review" as #201 and #190 are dependent on this (in that order) and there doesn't appear to be any lost functionality in either of those branches.

Fine with bundling all of them together into #190 as this and #201 are basically just organizational setup for it. I don't think it matters as much with these pre-1.0.0 releases but I'd like to at least get familiar with bundling PRs together into a single release just to have that skill under our belts.

@joshua-dean joshua-dean marked this pull request as ready for review October 9, 2024 13:14
Copy link
Member

@TrevorBurgoyne TrevorBurgoyne left a comment

Choose a reason for hiding this comment

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

looks g, everything still seems to work as well 👍

@joshua-dean joshua-dean mentioned this pull request Oct 11, 2024
6 tasks
@joshua-dean joshua-dean changed the base branch from main to rc/v0.16.0 October 11, 2024 13:58
@joshua-dean joshua-dean merged commit ae8a5e4 into rc/v0.16.0 Nov 8, 2024
@joshua-dean joshua-dean deleted the org/listeners branch November 8, 2024 18:23
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

Successfully merging this pull request may close these issues.

3 participants