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

Gl Animation Loss Blank Animation Fix #21040

Merged
merged 9 commits into from Mar 9, 2018
Merged

Conversation

epeach
Copy link

@epeach epeach commented Mar 6, 2018

Removes the special case for drawing your own animation.

Prev - don't save anything to V3, but save a reference in the manifest. When loaded, look for this special case and build a new Piskel.

New - Blank animation in the animation library. Draw-your-own calls this animation and then treats edits to it like a normal animation library change.

Blank Animation in animation library is 'deleted' and retrieved via version id so it won't appear in student-facing animation library. The animation was found by manually saving and downloading blank 'drawn' animation. Can be found: cdo-animation-library/category_backgrounds/blank.png

cc: @poorvasingal

return addLibraryAnimation(
{
name: 'animation',
sourceUrl: '/api/v1/animation-library/mUlvnlbeZ5GHYr_Lb4NIuMwPs7kGxHWz/category_backgrounds/blank.png',
Copy link
Contributor

Choose a reason for hiding this comment

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

😂 There's something wonderfully elegant about this. It also might not be 100% intuitive. Can we add a comment in addBlankAnimation() explaining this strategy of adding a secret, empty library animation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Example library animation props.

{
  "name": "bear",
  "frameCount": 1,
  "frameSize": {
    "x": 254,
    "y": 333
  },
  "looping": true,
  "frameDelay": 2,
  "jsonLastModified": "2017-04-18 05:56:09 UTC",
  "pngLastModified": "2017-04-18 05:56:10 UTC",
  "version": "vCJXAsYgUPWoNTt5h.OKY_tF3cEEY_Sf",
  "sourceUrl": "/api/v1/animation-library/vCJXAsYgUPWoNTt5h.OKY_tF3cEEY_Sf/category_animals/bear.png",
  "sourceSize": {
    "x": 254,
    "y": 333
  }
}

frameDelay: 4,
version: 'mUlvnlbeZ5GHYr_Lb4NIuMwPs7kGxHWz',
loadedFromSource: true,
saved: false
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect not all of these props are needed. Let's double check the format for library animation props and conform to that.

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

This change also depends on updating our Piskel dependency, yes?

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

My bad, didn't realize you'd already updated the Piskel dependency in #21093.

And I open the animation picker
And I select a blank animation
STEPS
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you also adding a UI test that uses these new steps?

Copy link
Author

Choose a reason for hiding this comment

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

Glad you pointed this out! I already wrote one but forgot to add it to the PR. Added in latest commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤣

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Made a couple suggested changes on my own, I think this is good to merge, UI test can follow.

@epeach epeach merged commit 0db7ce1 into staging Mar 9, 2018
@epeach epeach deleted the gl-anim-loss-blank-anim branch March 14, 2018 20:16
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

2 participants