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

Game Lab: S3-backed animation library #10685

Merged
merged 22 commits into from
Sep 21, 2016
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
496d850
Initial attempt at animation library manifest script
islemaster Sep 15, 2016
38f54b1
Add /api/v1/animation-library/... endpoint
islemaster Sep 15, 2016
fef2a8b
Add sourceSize to manifest
islemaster Sep 15, 2016
e34cc86
Include sourceUrl in animation library manifest
islemaster Sep 15, 2016
9faa59a
Animation library loads manifest, is searchable
islemaster Sep 15, 2016
330e458
Add bunny1 to animation library
islemaster Sep 16, 2016
7510134
Add progress bar to script
islemaster Sep 16, 2016
1d4768c
Suppress progress bars in --quiet mode
islemaster Sep 16, 2016
6b6ba94
Push event knowledge into AnimationPickerSearchBar
islemaster Sep 16, 2016
4c46c48
Variable locale import for gamelab
islemaster Sep 16, 2016
57d7551
Fix test - search returns Array, not Immutable object
islemaster Sep 16, 2016
2c84d26
Add file comment to animationLibrary.json
islemaster Sep 16, 2016
d37e904
Missed locale reference
islemaster Sep 16, 2016
c0fc1fd
Merge remote-tracking branch 'origin/staging' into animation-library
islemaster Sep 16, 2016
e9c9d71
Parallelize metadata generation
islemaster Sep 16, 2016
e2582bb
Extract common CLI utils
islemaster Sep 16, 2016
b9b2d77
Merge remote-tracking branch 'origin/staging' into animation-library
islemaster Sep 20, 2016
0967a97
Freeze ruby constants
islemaster Sep 21, 2016
d2711b4
Make style methods multiline
islemaster Sep 21, 2016
349af67
Clean up alias map builder per review feedback
islemaster Sep 21, 2016
145be7f
Fix unfinished commments
islemaster Sep 21, 2016
9042f84
Comment in CdoCli about keeping dependencies minimal
islemaster Sep 21, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 50 additions & 3 deletions apps/src/gamelab/AnimationPicker/AnimationPickerBody.jsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
/** Body of the animation picker dialog */
import React from 'react';
import Radium from 'radium';
import Immutable from 'immutable';
import color from '../../color';
import gamelabMsg from '../locale';
import animationLibrary from '../animationLibrary';
import animationLibrary from '../animationLibrary.json';
import ScrollableList from '../AnimationTab/ScrollableList.jsx';
import styles from './styles';
import AnimationPickerListItem from './AnimationPickerListItem.jsx';
import AnimationPickerSearchBar from './AnimationPickerSearchBar.jsx';

const MAX_SEARCH_RESULTS = 18;
Copy link
Contributor Author

@islemaster islemaster Sep 16, 2016

Choose a reason for hiding this comment

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

We are limiting the number of results to a query to avoid triggering a ton of asset loading while-you-type. Long-term I'd like to add pagination to search results so you can keep stepping through animations if you'd like.


const AnimationPickerBody = React.createClass({
propTypes: {
is13Plus: React.PropTypes.bool,
Expand All @@ -17,7 +20,18 @@ const AnimationPickerBody = React.createClass({
onUploadClick: React.PropTypes.func.isRequired
},

getInitialState() {
return {
searchQuery: ''
};
},

onSearchQueryChange(evt) {
this.setState({searchQuery: evt.target.value});
},

render() {
var pageOfResults = searchAnimations(this.state.searchQuery);
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we do this in the render function and not componentWillRecieveProps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, we are actually modifying the component's local state as the user types in the search box, which will not cause componentWillReceiveProps to occur. Conversely, we could call this from onSearchQueryChange but that wouldn't handle the initial render case so we'd need to do it in componentWillMount too... in the end, I think it's best to trust React to only call render() when it's needed (it's usually good at that) and optimize later if it becomes an issue. If you look at this component's props there's really nothing other than the search query that should trigger a re-render, so I suspect this will be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

coolio thanks for explaining!

Copy link
Contributor

Choose a reason for hiding this comment

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

Checking my understanding — page here just means the result set is limited, not that there's pagination involved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It was optimistic naming on my part, since I want pagination in the future and could picture searchAnimations taking a 'start at index' argument... but I can rename if this is confusing.

return (
<div>
<h1 style={styles.title}>
Expand All @@ -28,7 +42,10 @@ const AnimationPickerBody = React.createClass({
{gamelabMsg.animationPicker_warning()}
</WarningLabel>
}
<AnimationPickerSearchBar />
<AnimationPickerSearchBar
value={this.state.searchQuery}
onChange={this.onSearchQueryChange}
/>
<ScrollableList style={{maxHeight: 400}}> {/* TODO: Is this maxHeight appropriate? */}
<AnimationPickerListItem
label={gamelabMsg.animationPicker_drawYourOwn()}
Expand All @@ -40,7 +57,7 @@ const AnimationPickerBody = React.createClass({
icon="upload"
onClick={this.props.onUploadClick}
/>
{animationLibrary.map(animationProps =>
{pageOfResults.map(animationProps =>
<AnimationPickerListItem
key={animationProps.sourceUrl}
label={`${animationProps.name} (${animationProps.frameCount})`}
Expand All @@ -63,3 +80,33 @@ export const WarningLabel = ({children}) => (
WarningLabel.propTypes = {
children: React.PropTypes.node
};

/**
* Given a search query, generate a results list of animationProps objects that
* can be displayed and used to add an animation to the project.
* @param {string} searchQuery - text entered by the user to find an animation
*/
function searchAnimations(searchQuery) {
// Make sure to generate the search regex in advance, only once.
// Search is case-insensitive
// Match any word boundary or underscore followed by the search query.
// Example: searchQuery "bar"
// Will match: "barbell", "foo-bar", "foo_bar" or "foo bar"
// Will not match: "foobar", "ubar"
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be interesting to test with the animation names we will use to see if we need a different type of search. It'll also be interesting to test if kids have ideas of what to search or if we'll need to add promps or suggestions. (I just read the spec said 1,000 - 10,000 images which is a ton)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 1000-10000 was my own ballpark, I'm not sure what the content team has in mind. I think kids will know what to search for but there is a discoverability concern - I was actually wondering how we should special-case the results when there is no search query. Right now we always display the first n animations (alphabetical) but I'm wondering if we should instead pick a random set, or highlight the newest content.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is what Caley was getting at but just in case not — it would be awesome to somehow save out failed searches (once user backtracks when no results?) to see what graphics students wish we had (& potential mis-searches). I know of at least one coding website that has done that as a way to source graphic additions and kids loved it (really makes it feel like the website offers something for everything).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow! That's an awesome idea, but maybe another pull request. We're doing similar logging when we strip unexpected markup from applab design mode HTML to see what we might be mis-categorizing. I'll take a look at this.

const searchRegExp = new RegExp('(?:\\b|_)' + searchQuery, 'i');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: I suppose I should escape searchQuery somehow? I mean, it's pretty cool to support regex in our search, but probably not helpful to students.


// Generate the set of all results associated with all matched aliases
const resultSet = Object.keys(animationLibrary.aliases)
.filter(alias => searchRegExp.test(alias))
.reduce((resultSet, nextAlias) => {
return resultSet.union(animationLibrary.aliases[nextAlias]);
}, Immutable.Set());
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'm trying out Immutable.JS Set here, so I don't have to think about removing duplicates. Not really sure if there's a performance benefit.


// Finally alphabetize the results (for stability), take only the first
// MAX_SEARCH_RESULTS so we don't load too many images at once, and return
// the associated metadata for each result, along with
Copy link
Contributor

@bcjordan bcjordan Sep 21, 2016

Choose a reason for hiding this comment

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

unfinished comment

return resultSet
.sort()
.slice(0, MAX_SEARCH_RESULTS)
.map(result => animationLibrary.metadata[result]);
}
12 changes: 11 additions & 1 deletion apps/src/gamelab/AnimationPicker/AnimationPickerSearchBar.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,20 @@ var styles = {
};

var AnimationPickerSearchBar = React.createClass({
propTypes: {
value: React.PropTypes.string.isRequired,
onChange: React.PropTypes.func.isRequired
},

render: function () {
return (
<div>
<input style={styles.input} placeholder="Search for images" />
<input
style={styles.input}
placeholder="Search for images"
value={this.props.value}
onChange={this.props.onChange}
/>
<button style={styles.button}><i className="fa fa-search"></i></button>
</div>
);
Expand Down
75 changes: 0 additions & 75 deletions apps/src/gamelab/animationLibrary.js

This file was deleted.

Loading