Skip to content

fix: Toggle Event Effect Chooser Data Typing#3008

Merged
ebiggz merged 2 commits intocrowbartools:v5from
phroggster:fix/3006
Feb 20, 2025
Merged

fix: Toggle Event Effect Chooser Data Typing#3008
ebiggz merged 2 commits intocrowbartools:v5from
phroggster:fix/3006

Conversation

@phroggster
Copy link
Collaborator

@phroggster phroggster commented Feb 20, 2025

Description of the Change

  • Loading a dropdown-select element with no values available to bind to it previously locked it into string-handling mode.
  • This broke the Toggle Event effect on initial creation, which expected object-handling mode, but supplied no initial binding values.
    • As such, the initially saved effect data used the name of the event, instead of the unique ID of it.
  • Added an optional one-way "value-mode" attribute to dropdown-select to force it to operate in either object or string mode, or default to prior behavior when unspecified.
  • Fixed two arrow-parens.
  • Beyond original scope:
    • Have the effect's group name track renaming of event sets, which would cause similar problems in the effect's UI.

Applicable Issues

#3006

Testing

Tested Toggle Event creation and editing thoroughly, plus a handful of others. I still need to verify whether a similar issue could occur in approximately 4 other places, at which point I'll just tack them on to here sometime tonight.

Screenshots

Newly-created effect before:

{"id":"c99f52f9-2875-48e5-b1c2-3295988887a1","type":"firebot:toggle-event","active":true,"toggleType":"disable","selectedGroupName":"General Events","selectedEventId":"Sub"}

Newly-created effect after:

{"id":"1fad718c-0da3-4416-aeff-d9894407f0e9","type":"firebot:toggle-event","active":true,"toggleType":"toggle","selectedGroupName":"General Events","selectedEventId":"0eaf3180-ae07-11ed-a61c-31cd9d099aeb"}

- Loading a dropdown-select element with no values loaded to bind to it
  locked it into string-handling mode.
- This broke the Toggle Event effect on initial creation, which expected
  object-handling mode, but supplied no initial binding values.
- Added an optional one-way "value-mode" attrib to dropdown-select to
  force it to operate in either object or string mode, or default to
  prior behavior when unspecified.
- See crowbartools#3006
@phroggster phroggster requested a review from ebiggz as a code owner February 20, 2025 06:15
@phroggster
Copy link
Collaborator Author

It looks like the other locations where a similar issue could have potentially been reached all have other means of preventing it from occurring. Either length checking, hard-coded choices, or no potential way to initialize the control in an empty manner, then later show it.

That makes me re-question the approach I took here, but this PR does keep the UX identical while avoiding the issue. It also allows other places to be able to change the value mode in their template should they desire to, so I'm keeping it. Let me know if you'd prefer changing the UX of this one effect instead of the added attribute, or if you've got a better idea to deal with this.

Copy link
Member

@ebiggz ebiggz left a comment

Choose a reason for hiding this comment

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

Thanks for this fix - getting into the weeds with angularjs' archaic attribute data binding is never a fun time so I empathize.

@ebiggz ebiggz merged commit ade4aa7 into crowbartools:v5 Feb 20, 2025
1 check passed
@phroggster phroggster deleted the fix/3006 branch February 20, 2025 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments