Skip to content

Animation Tab Simple Upload#7905

Closed
islemaster wants to merge 18 commits into
stagingfrom
animation-upload
Closed

Animation Tab Simple Upload#7905
islemaster wants to merge 18 commits into
stagingfrom
animation-upload

Conversation

@islemaster

Copy link
Copy Markdown
Contributor

Establishes the infrastructure to allow uploading PNGs to an 'animations' bucket on S3 and associate animation metadata with the project source. Uploaded images show up in the left column and can be renamed, cloned, and deleted.

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.

This PR includes server and client changes.

Server

We've added a v3/animations API that is based on the assets and sources APIs. I manually created the S3 bucket for this API already. It handles file uploads, and also access to particular versions.

Client

Added real upload functionality to the AnimationPicker, store animation metadata along as part of the versioned info going to the 'sources' API, enable adding animations via upload, deleting animations, cloning animations and renaming animations.

Note that for now, "animations" are all simple PNGs, single-frame. We'll add sprite-sheet support as things get more complicated.

Animations are given a UUID "key" independent of their name, which becomes their object key on S3. This lets the student freely rename them.

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.

Comment thread apps/package.json
"react-redux": "^4.4.0",
"redux": "^3.3.1",
"redux-logger": "^2.6.1",
"redux-thunk": "^2.0.1",

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.

redux-thunk allows your action-creators to return functions instead of POJOs. Those functions in turn are able to read state and dispatch additional actions. This provides a neat template for dispatching asynchronous actions, and is a recommended way to handle async work in redux.

@islemaster

Copy link
Copy Markdown
Contributor Author

Brent, PTAL - and feel free to suggest ways to break this apart into smaller PRs. I suppose I could pull all the server stuff out and merge it first.

@islemaster

Copy link
Copy Markdown
Contributor Author

Brent, never mind, I know I need to break this up. I'd appreciate any thoughts you have on the broad approach though.

@islemaster islemaster closed this Apr 15, 2016
return <input
ref="uploader"
type="file"
style={{display: 'none'}}

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.

could use commonStyles.hidden here if desired.

this.setState({
view: View.UPLOAD_IN_PROGRESS,
originalFileName: data.files[0].name
});

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.

In a world where we have redux, it may make sense to have a discussion about if/where we still use local state. I think it would be good if we had a clear store on when it was reasonable.

@Bjvanminnen

Copy link
Copy Markdown
Contributor

Gave things an initial look. My two high level concerns are (1) functions in our store/reducers and (2) When to use local state in a redux world. Let me know if there are other specific things you wanted feedback on.

@islemaster

Copy link
Copy Markdown
Contributor Author

Opened new API changes (only) in #7911.

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