Allow multiple beacons with the same name #458
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…when a single beacon is present
|
@mattmarcum could you please fix lint? This looks good to me, but I'd like @ef4 to chime in to confirm this is the right fix. |
SergeAstapov
requested changes
Apr 22, 2022
ef4
approved these changes
Apr 22, 2022
…pose Adding a hasBeacon method to the -ea-motion service to guard against costly recalculations
|
pushed up lint fixes |
SergeAstapov
approved these changes
Apr 22, 2022
SergeAstapov
changed the title
|
@mattmarcum this is now published to npm as |
|
I tried that npm package locally and can confirm the errors no longer flood the console. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses #225
First commit I added a failing test that just had a beacon and two animated-ifs. That test fails like the bug describes.
After diving into the code a bit I feel that this assertion was put here in error. As the added test shows, when multiple animated components are present the beacon will add itself to the -ea-motion service once per new animation.
This is because the beacons are listening to all animations. The original authors didn't cover the case when multiple animations would start in the same cycle, i.e. when more than one animated component gets rendered on the page.
I added a
hasBeaconmethod so that the Beacon component can return early if it's already added itself to the service.The problem the original author tried to help with, warning when devs add two similar named beacons, is tough to crack with this implementation. I think its better to just fix this error and fore-go the warning.
One thing to note, the original error didn't really impede animations. Throwing an error would effective cause the
addBeacontask to exit early and the sprite never got updated.