add loop and loopMode capabilities for AnimationSystem, fix for comp-audio to match comp-anim: state-->action

[hanbollar]

Linking

related to:

• loop not impl
• audio state --> action for consistency

Problem

loop not implemented for data-comp-animation

Solution

implement it

Breaking Change

YES!!

• changes audio state --> action. Made sure this was updated in the documentation repo and the samples here
• adds looping for data-comp-animation

Notes

...

---

Required to Merge

• PASS - all necessary actions must pass (excluding the auto-skipped ones)
• TEST IN HEADSET - main dev-testing-example and any of the other examples still work as expected
• VIDEO - if this pr changes something visually - post a video here of it in headset-MR and/or on desktop (depending on what it affects) for the reviewer to reference.

Untitled.mov

• TITLE - make sure the pr's title is updated appropriately as it will be used to name the commit on merge
• BREAKING CHANGE
  • DOCUMENTATION: This includes any changes to html tags and their components
    • make a pr in the documentation repo that updates the manual docs to match the breaking change
    • link the pr of the documentation repo here: add data-comp-animation page and update audio for 'state'-->'action' documentation#50
    • that pr must be approved by @lobau
  • SAMPLES/INDEX.HTML: This includes any changes (html tags or otherwise) that must be done to our landing page submodule as an effect of this pr's updates
    • make a pr in the mrjs landing page repo that updates the landing page to match the breaking change
    • link the pr of the landing page repo here: #pr
    • that pr must be approved by @hanbollar

[render]

Your Render PR Server URL is https://examples-mrjs-pr-599.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-coo4ia7sc6pc73a8rg90.

[michaelthatsit]

Looks great with a few comments!

[michaelthatsit]

You can also do comp?.action which is a little cleaner.

[hanbollar]

i like the simpler approach you note, but that also makes it turn into a truthy check of the action value if it exists, meaning it can silently error (instead of hitting the default case) if a user makes a typo and leaves it as a 0 or an empty string for example

i'd prefer to keep it as the hasOwnProperty line, but lmk your thoughts

[michaelthatsit]

hmm does comp?.action == null not hit the default state?

[hanbollar]

sorry im confused - all that does is check if comp exists - and then whether comp.action == null

in the comp.hasOwnProperty line im trying to check if action exists before running through our switch statements - do you have a different preferred way of writing that?

in the audio system it looks like that's setup as comp.action ?? false - is that what you'd prefer instead?

[hanbollar]

i think i'd still be a little worried of the comp.action value existing but being incorrect as a truthy-ness option and not going into if-statement even with comp.action ?? false 🤔

thoughts?

[hanbollar]

i'm going to merge it in since approved so not blocker, but lmk since that's a quick pr fix afterwards if needed

[michaelthatsit]

Approved for merge.

I use object.property pretty much everywhere so we miiight need to decide which approach to take across the board for consistency at some point.

[hanbollar]

I use object.property pretty much everywhere so we miiight need to decide which approach to take across the board for consistency at some point.

oh i think object.property is fine most places we have it - i just think this is an edge situation where we need to be very careful of user entry given the multiple options that arent just true/false