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

Animation widget #458

Merged
merged 182 commits into from
Feb 25, 2013
Merged

Animation widget #458

merged 182 commits into from
Feb 25, 2013

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Jan 11, 2013

New widget for controlling time, and updated API in Clock/AnimationController to match new actions offered by the widget.

Conflicts:
	Apps/TimelineDemo/TimelineDemo.js
…ay or may not have resulted from auto-upgrading to Firefox 17).
…w (although touch events have yet to be added, awaiting new event system from camera/pinch-zoom branches).
Conflicts:
	Source/Widgets/Dojo/CesiumViewerWidget.js
…R, added a realtime mode, added loop option to widget.
All tests currently pass in Chrome 23 with ANGLE on.
@shunter
Copy link
Contributor

shunter commented Feb 21, 2013

For auto-snapping, I was briefly looking at:

http://html2canvas.hertzen.com/

I don't think we need to redo all the thumbnails. Even before this change, many of them didn't match anyway.

throw new DeveloperError('timeFormatter must be a function.');
}
this._timeFormatter(timeFormatter);
this._timeFormatter.notifySubscribers();
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that this is to work around the default equalityComparer which only works with primitive types. Should we use a different equalityComparer instead of manually notifying?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it was from an earlier version of the code and is no longer needed. I removed this and the other _dateFormatter.notifySubscribers call.

@mramato
Copy link
Contributor

mramato commented Feb 22, 2013

I was on that same site today. I even went as far to install grunt and cloned and built it. Haven't tried to use it yet since I realized we need to update other things, like good default views for each example.

@mramato
Copy link
Contributor

mramato commented Feb 22, 2013

I think all of your review comments have been addressed (either my changing the code or in my responses above). If there's anything else, let me know. Otherwise, merge when you're ready.

@mramato
Copy link
Contributor

mramato commented Feb 22, 2013

I realized CHANGES was out of date, so I updated it with details of the huge amount of things that happened in this pull. Ready for "final" review if you are.

@kring
Copy link
Member

kring commented Feb 22, 2013

Guys, I looked at this briefly and it seems reasonable to me. Unless you think it would be worthwhile for me to do a more extensive review, you can put me down in the "merge it" column.

@mramato
Copy link
Contributor

mramato commented Feb 22, 2013

Good enough for me. @shunter merge when ready.

shunter added a commit that referenced this pull request Feb 25, 2013
@shunter shunter merged commit c692130 into master Feb 25, 2013
@shunter shunter deleted the playback branch February 25, 2013 21:46
@mramato
Copy link
Contributor

mramato commented Feb 25, 2013

@emackey Do you want to write a blog entry that gives an overview of the new Animation widget and also talks about using SVG and other modern UI/HTML design? You could also highlight some of the hiccups you encountered and get some traction for the two HTML5 blob bugs you submitted. It would be nice if we had something up at the end of the week to go along with the b14 release. Do you have time to put something together? If not, I can look into doing it and you can just proof it.

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.

5 participants