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

Adds the play button to the slider #407

Merged
merged 1 commit into from May 22, 2015

Conversation

Projects
None yet
3 participants
@anmoljagetia
Contributor

anmoljagetia commented May 21, 2015

This is in reference to #353.
@GordonSmith @mlzummo Please review.
This adds a play and a loop button to the slider.

The buttons don't appear as of now on the rawgit link, becuase the styles override the FontAwesome style with Verdana. #406 fixes this issue.

Demo at : http://rawgit.com/anmoljagetia/Visualization/playSlider2/demos/dermatology.html

Also, I am looking for a better commit message, and squashing all the commits into one? does "Adds the ability to play or loop GH-353" look plausible?

@@ -42,6 +42,7 @@
<li>
<prompt>Widget:</prompt>
<select id="widgetSelect" onchange="testWidget(this.value);">
<option value="src/other/Slider">Slider</option>

This comment has been minimized.

@GordonSmith

GordonSmith May 22, 2015

Member

Move this down to the other "Other" widgets,

@@ -35,3 +35,28 @@
stroke-width: 1.00px;
pointer-events: none;
}
.common_Icon:hover {

This comment has been minimized.

@GordonSmith

GordonSmith May 22, 2015

Member

Should these styles be in the common/Icon.css file?

This comment has been minimized.

@anmoljagetia

anmoljagetia May 22, 2015

Contributor

Actually they are only meant to work on the icon here, I don't want to override the default icon settings, in the main icon styles file.

};
Slider.prototype.stop = function () {
console.log("stopped");

This comment has been minimized.

@GordonSmith

GordonSmith May 22, 2015

Member

Remove all console messages (except the default click)

this.xScale
.domain([this.low(), this.high()])
.range([-width/2, width/2])
;
this.data(this._data);

This comment has been minimized.

@GordonSmith

GordonSmith May 22, 2015

Member

Your setting the current data with the current data?

.domain([this.low(), this.high()])
.range([-width/2, width/2 - this._icon.diameter()*2 - this.playGutter()])
;
this.data(this._data);

This comment has been minimized.

@GordonSmith

GordonSmith May 22, 2015

Member

Your setting the current data with the current data?

@@ -109,20 +198,67 @@
.attr("class", "handle")
.attr("transform", "translate(0,-27)")
;
this._icon

This comment has been minimized.

@GordonSmith

GordonSmith May 22, 2015

Member

These should be in the enter method.

@anmoljagetia anmoljagetia force-pushed the anmoljagetia:playSlider2 branch 3 times, most recently from 5ae685d to e635b76 May 22, 2015

@GordonSmith

This comment has been minimized.

Member

GordonSmith commented May 22, 2015

Looks fine.

@mzummo

This comment has been minimized.

Contributor

mzummo commented May 22, 2015

LGTM

@GordonSmith

This comment has been minimized.

Member

GordonSmith commented May 22, 2015

In your commit you had "Fixes GH #353" - I am not sure if that will auto close the issue, I know:
Fixes GH-353
and
Fixes #353
Will.

Adds the ability to play or loop to Slider GH-353
Fixes #353

Signed-off-by: Anmol Jagetia <anmoljagetia@gmail.com>

@anmoljagetia anmoljagetia force-pushed the anmoljagetia:playSlider2 branch from e635b76 to 9e072f5 May 22, 2015

GordonSmith added a commit that referenced this pull request May 22, 2015

Merge pull request #407 from anmoljagetia/playSlider2
Adds the play button to the slider

@GordonSmith GordonSmith merged commit 67716ac into hpcc-systems:master May 22, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@anmoljagetia anmoljagetia deleted the anmoljagetia:playSlider2 branch May 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment