Spritelab/Gamelab Animation Library changes for backgrounds #36578
Conversation
…k-background-logic
…k-background-logic
| newResults = newResults.filter( | ||
| animation => !animation.categories.includes('backgrounds') | ||
| ); |
There was a problem hiding this comment.
nit: we filter backgrounds in multiple places now, so we should pull this out into a helper method
also, question: this will currently filter a result that has ["backgrounds", "environments"] as its categories -- is that what we want? or should we only filter the result if its only category is "backgrounds"?
There was a problem hiding this comment.
I think that we should filter it out if it has backgrounds as a category because we don't want them to be able to use a background as a sprite right?
| onClick={this.onClearCategories} | ||
| style={animationPickerStyles.allAnimations} | ||
| > | ||
| {`${msg.animationPicker_allCategories()} > `} |
There was a problem hiding this comment.
we should move the carrot into the animationPicker_allCategories string so that it displays correctly in RTL languages (right now, the greater-than sign would be on the incorrect end of the string)
There was a problem hiding this comment.
Elijah asked me to take it out, do you still want me to put it back in? #36578 (comment)
There was a problem hiding this comment.
ah, sorry, i missed his comment -- he's our resident i18n expert, so i'll take his recommendation 😄
There was a problem hiding this comment.
The browser is (usually) capable of aligning things like this correctly in RTL, and this way we avoid confusing translators with non-language characters
| }) | ||
| ); | ||
| // if checkBackground, this means we don't want the selected animation to be a background | ||
| if (!checkBackground || !props.categories.includes('backgrounds')) { |
There was a problem hiding this comment.
nit: maybe skipBackground instead of checkBackground?
now that we have this param, do we need to check props.categories? or can we just manipulate the param?
There was a problem hiding this comment.
we need to check props.categories to decide whether or not to set selected in the costume library. Otherwise, spritelab would never have default selected sprites showing up in the costume library (they'd have to scroll a bunch)
apps/src/p5lab/spritelab/blocks.js
Outdated
| if (backgrounds) { | ||
| return ( | ||
| animation.categories && animation.categories.includes('backgrounds') | ||
| ); | ||
| } else { | ||
| return !( | ||
| animation.categories && animation.categories.includes('backgrounds') | ||
| ); | ||
| } |
There was a problem hiding this comment.
nit: i think we can simplify this filter function as well (definitely double-check my logic here)
| if (backgrounds) { | |
| return ( | |
| animation.categories && animation.categories.includes('backgrounds') | |
| ); | |
| } else { | |
| return !( | |
| animation.categories && animation.categories.includes('backgrounds') | |
| ); | |
| } | |
| return backgrounds && (animation.categories || []).includes('backgrounds') |
There was a problem hiding this comment.
the logic didnt quite work but I tried simplifying it, let me know what you think
apps/src/p5lab/spritelab/blocks.js
Outdated
| import i18n from '@cdo/locale'; | ||
| import spritelabMsg from '@cdo/spritelab/locale'; | ||
|
|
||
| function sprites() { | ||
| function animations(backgrounds) { |
There was a problem hiding this comment.
it isn't clear what this param does by its name -- maybe skipBackgrounds again? or includeBackgrounds if we want the inverse (also open to suggestions)
There was a problem hiding this comment.
how about isBackgroundAnimations?
There was a problem hiding this comment.
ah, i still misunderstood the param meaning lol. i think that's fine, or even just areBackgrounds since the function is called animations and now the param will communicate that it should be a boolean
This sets up everything we will need to get backgrounds safely included as part of the animation library in sprite lab without letting them show up when students are trying to add sprites in addition to adding a new block input backgroundPicker that will be used in a new set background as block. This PR should not change the behavior of any animation stuff in sprite or gamelab.
Links
Testing story
Added the extra fields to animation picker body tests and wrote tests for the actions show and showBackground in animation picker module.
Reviewer Checklist: