-
Notifications
You must be signed in to change notification settings - Fork 865
New exercise: Interactive telling time #8913
Conversation
…, added fixedDistance snap to interactive.js
Oh dang, I just saw that someone else attempted this already: #6362. But it seems they didn't finish... |
Whoops, I didn't mean to commit the changes to |
Nice exercise @stchangg!! Great addition to the time setting series. Will be mentioning a few minor suggestions - will take a deeper look in the next few days. |
<div id="set-hands" data-weight="3"> | ||
|
||
<div class="vars"> | ||
<var id="HOUR">randRange( 1, 12 )</var> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time should never === 1:00 [default time shown on clock] - data-ensure should make sure this won't happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just made this change... not entirely sure that this is the right way to use data-ensure, but after checking the other exercises, it seems OK to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if you had the clock randomly set to a time and make sure it doesn't match what the user is supposed to set it to? The idea of eliminating one time from the set seems wrong. Or you could make having the time match the answer a gimme
graph.minuteSnapDegrees = 360 / minuteSnapPoints; | ||
graph.hourSnapDegrees = 360 / hourSnapPoints; | ||
|
||
graph.clock = KhanUtil.addAnalogClock( { radius: clockRadius, minuteTicks: hourSnapPoints } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within a graphie div like this, you shouldn't have to prefix stuff with KhanUtil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, did not realize. Removed them. Question: does this also apply to the "validator-function" div, or do I need to use KhanUtil there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. You shouldn't need it in validator-function either. Generally if you need to reference KhanUtil it anywhere in the html file, something is probably wrong.
I modified the hints and felt that the |
I'm pretty sure I made all of the latest requested changes. Let me know if there's anything left! |
… graph. prefix from variables in telling_time_interactive
@beneater I refactored the other time exercises and removed the old time code from graphie-helpers. :) |
if ( this.showLabels ) { | ||
this.drawLabels(); | ||
} | ||
if ( this.hour && this.minute ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be if ( this.hour !== undefined && this.minute !== undefined ) {
, so this doesn't happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, fixed this.
if ( this.showLabels ) { | ||
this.drawLabels(); | ||
} | ||
if ( this.hour !== undefined && this.minute !== undefined ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hrm.. i just noticed it's drawing the hands at 12:00 in the interactive exercise now, so maybe these are defined but some other kind of falsey value? Haven't looked through to see exactly what's going on (you can probably figure it out faster than me), but something about this check isn't working right in the interactive version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I made the change too hastily. This probably should be checking for false, not undefined, since I set default values for those params to false when instantiating the analogClock object. Just pushed the fix.
…ues instead of undefined before drawing hands in time.js
Awesome! This is a really great exercise! As I mentioned before, it might be another week or so before it goes live since Matt's out of town :( |
New exercise: Interactive telling time
Thanks @stchangg for rocking this - very slick. Will let you know when we push live later this week/weekend! |
Trello bug: "Telling time, interactive" in "Queued up"
Feedback much appreciated!
CHANGED
telling_time_interactive.html
time.js
analogClock()
fromgraphie-helpers.js
to a more customizableaddAnalogClock()
function (modeled afteraddMovablePoint()
) insidetime.js
coordToDegrees()
anddegreesToCoord()
may belong in other packages. Any ideas where?roundToNearest()
belongs inmath.js
. See below.interactive.js
snapPoints
option tomovablePoint.constraints.fixedDistance
to allow snapping around a circle (fixedDistance
restricts the point's motion to a circle).snapPoints
is the number of points that can be snapped to around the circle.angles.js
andtime.js
, but this might be unavoidable sinceinteractive.js
may be used without the others.angle.js
toDegrees()
functionNO CHANGE CURRENTLY, BUT SHOULD CHANGE
telling_time.html & telling_time_0.5.html
time.js
package (if accepted)math.js
roundToNearest()
(currently intime.js
) tomath.js
but for some reason I couldn't get it to work. Something about howmath.js
is compiled? Is it a special file compared to the others?graphie-helpers.js
time.js
is accepted andtelling_time.html
&telling_time_0.5.html
are updated, we should remove theanalogClock()
function from here.