Skip to content

Animation Tab: Upload PNGs#7959

Merged
islemaster merged 46 commits into
stagingfrom
upload-pngs
Apr 22, 2016
Merged

Animation Tab: Upload PNGs#7959
islemaster merged 46 commits into
stagingfrom
upload-pngs

Conversation

@islemaster

Copy link
Copy Markdown
Contributor

Add the ability to upload a PNG image to the animation tab, and rename, clone or delete that image. Uses the new v3/animations API added in #7911. This is the last extracted part of #7905.

This is just the first step along the road to an actual functioning Animation Tab. The next step will be using the basic metadata we have to build a createSprite(animationLabel, x, y) command, so the curriculum team can start to build around assets imported through the animation tab. Then we'll turn our attention to integrating Piskel, which will make the animation tab functionality much more robust.

Possible Improvments

I'd appreciate some help working out what's important to do now, and what I can save for later. Again, we want to get a simple version of this to the Ed team ASAP.

  • Undo-delete: Right now deletes are an immediate, "permanent" operation, which isn't user-friendly. Could add a placeholder 'confirm' I suppose.
  • Expect success: Clone and delete operations are 'slow' now because they require a server request, and they don't update UI until they complete. More work is needed to produce a nice intermediate state, or to pretend immediate success for responsiveness and 'back out' if the request fails.
  • No Versioning: Some groundwork has been laid here for versioning animations along with sources (including storing the S3 version key in the metadata) but we don't currently do anything to support restoring an animation version.
  • I haven't implemented particularly useful error states/messages yet - still doing some console logging in failure cases.

}
}

function onComplete(state, action) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TODO: Don't put functions in the store.

@islemaster

Copy link
Copy Markdown
Contributor Author

Brent, PTAL. I believe I've addressed everything from your initial scan of the original PR, and generally done a lot of cleanup.

type: HANDLE_UPLOAD_ERROR,
status: status
};
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like how this turned out as a duck module.

In terms of what we export, we should probably try to be consistent (and document somewhere what expectations are). It looks like you've exported a reducer, and an object of actions. https://github.com/erikras/ducks-modular-redux recommends exporting a reducer as default, and then exporting actions as top level functions (this is largely inspired by ES6 syntax where I can then do import reducer from './duckModule and import * as actions from './duckModule.

I'm not sure I have strong opinions on what is best. Your way seems fine if we want to make this the standard.

@Bjvanminnen

Copy link
Copy Markdown
Contributor

lgtm :)

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.

2 participants