Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Sprite Lab] Store selected animation for costume/background mode #50050

Merged
merged 28 commits into from
Feb 9, 2023

Conversation

mikeharv
Copy link
Contributor

@mikeharv mikeharv commented Feb 2, 2023

Builds up upon the new Sprite Lab Backgrounds tab (Experiment):

Interesting/digestible commits on this branch:

  • 29fecd9 - simplify reducer
  • 72b4f24 - change current animation state from string to object
  • d492c42 - store selected animation for costume/background mode

Overview

Costumes and backgrounds are SpriteLab-specific terms that both refer to "animations". This branch adds onto the initial work to create a Backgrounds tab in Sprite Lab for managing backgrounds (animations assigned to the "backgrounds" category). Specifically, this work presents a potential strategy for storing the currently selected animation (the one being edited in Piskel) separately between the existing Costumes tab and the new Backgrounds Tab.

Previously, there was only one animation key (string) stored as the selectedAnimation and the costumes and background modes both shared it. Now, we have a currentAnimations object that stores on key for each tab. The 'default' key refers to the selected animation in Game Lab, or the selected costume in Sprite Lab. The 'background' key refers to the selected background, which is only in Sprite Lab. Note: This property was renamed to distinguish it from a similar-sounding selectedAnimations property which is only used in the AnimationPicker (where students go to add one or multiple new animations from the library to their project).

This new state object had to be configured to work not only with selecting animations (to open for editing with Piskel), but also cloning, deleting, and adding - we update the current animations as part of each of these actions.

Video demonstrates the new behavior where a separate state is maintained for each tab.

2023-02-02.15-14-36.2023-02-02.15_18_21.mp4

Links

Jira - https://codedotorg.atlassian.net/browse/SL-52

Testing story

Relevant tests were updated to use the correct structure for the Animation Tab's initial state. New unit tests were added to check for correct selection of background animations: 1851d71

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@mikeharv mikeharv marked this pull request as ready for review February 3, 2023 21:22
@mikeharv mikeharv requested a review from a team February 3, 2023 21:23
apps/src/p5lab/AnimationTab/PiskelEditor.jsx Outdated Show resolved Hide resolved
apps/src/p5lab/constants.js Outdated Show resolved Hide resolved
apps/src/p5lab/redux/animationList.js Outdated Show resolved Hide resolved
apps/src/p5lab/redux/animationList.js Outdated Show resolved Hide resolved
@mikeharv
Copy link
Contributor Author

mikeharv commented Feb 8, 2023

@madelynkasula @molly-moen Please see 579bdce
I combined the requests from your most recent feedback to just use the current P5LabInterfaceMode to determine the current animation type we want to use. This refactor removes one enum entirely and hopefully makes things clearer and cleaner overall.

Copy link
Contributor

@molly-moen molly-moen left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for dealing with all the churn

dispatch(loadAnimationFromSource(key, () => {}));
}
const isSpriteLabBackground =
props.categories?.[0] === 'backgrounds' && isSpriteLab;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you add a comment explaining that if we are in background mode the categories list will be a single item list containing the category backgrounds?

Copy link
Contributor Author

@mikeharv mikeharv Feb 8, 2023

Choose a reason for hiding this comment

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

The code here controls adding a new animation to the sidebar on the left. This code is used to determine if now we should be selecting a animation or a background based on the category information for the animation that was chosen.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I wasn't super clear--I was thinking of a comment explaining why props.categories?.[0] === 'backgrounds' will always tell you if it is a background or not. The [0] feels a bit magical

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this to be a an .includes to make it a little safer and less magical. In practice, the backgrounds category is mutually exclusive from all other categories, but that's definitely a bit of a fragile system.

Copy link
Contributor

@maddiedierker maddiedierker left a comment

Choose a reason for hiding this comment

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

really great work, mike! thank you for working through this with us!

@mikeharv mikeharv merged commit 67934d5 into staging Feb 9, 2023
@mikeharv mikeharv deleted the mike/add-state-for-selected-background branch February 9, 2023 19:09
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.

None yet

3 participants