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

Formal click event for Icon #404

Merged
merged 1 commit into from May 21, 2015
Merged

Conversation

@anmoljagetia
Copy link
Contributor

@anmoljagetia anmoljagetia commented May 20, 2015

addressing #384.

@anmoljagetia anmoljagetia force-pushed the anmoljagetia:iconClick branch from 52c923d to 8b93102 May 20, 2015
@GordonSmith
Copy link
Member

@GordonSmith GordonSmith commented May 20, 2015

Can you take a look at: https://github.com/hpcc-systems/HPCC-Platform/wiki/Git-commit-message-guidelines Which is a rough guide for commit messages.

@GordonSmith
Copy link
Member

@GordonSmith GordonSmith commented May 20, 2015

I rebased this pull request onto master (without the merge) here: https://github.com/GordonSmith/Visualization/commits/iconClick if you can take that and ammend the commit with a better msg that would be great.

@@ -44,6 +44,11 @@
.render()
;
this._tooltipElement = element.append("title");
this.element().on("click", this.click);

This comment has been minimized.

@GordonSmith

GordonSmith May 20, 2015
Member

I think element.on("click", this.click); is equivalent.

@mzummo
Copy link
Contributor

@mzummo mzummo commented May 20, 2015

are we going with onEvent(event,callback) ?

@anmoljagetia anmoljagetia force-pushed the anmoljagetia:iconClick branch from 8b93102 to dbf43ef May 20, 2015
@anmoljagetia
Copy link
Contributor Author

@anmoljagetia anmoljagetia commented May 20, 2015

So should I change the messsage to GH384 - Adds a formal click event to icon, or anything better?

@mzummo
Copy link
Contributor

@mzummo mzummo commented May 20, 2015

Do you have a chat client or can you jump on gitter.im?

@mzummo
Copy link
Contributor

@mzummo mzummo commented May 20, 2015

@anmoljagetia "Adds Formal Click Event #384"

@GordonSmith
Copy link
Member

@GordonSmith GordonSmith commented May 21, 2015

@mlzummo The click handler here is similar to all the other click handlers, (but we will have a discussion about "publishing" events at some point in the future).

@anmoljagetia - If you can change your commit message to adhere to the recommended format I will pull:

GH-384 Icon Click Event

Add a click event to the common/Icon.

Fixes GH-384

Signed-off-by: Anmol Jagetia anmoljagetia@lexisnexis.com

Note: you get the signature via git with: git commit -s

@anmoljagetia anmoljagetia force-pushed the anmoljagetia:iconClick branch from dbf43ef to 5e50edb May 21, 2015
@@ -44,6 +44,11 @@
.render()
;
this._tooltipElement = element.append("title");
element().on("click", this.click);

This comment has been minimized.

@GordonSmith

GordonSmith May 21, 2015
Member

should be element.on("click", this.click);

@GordonSmith
Copy link
Member

@GordonSmith GordonSmith commented May 21, 2015

Also when issuing the PR - it would be very helpful if you could add a link to the test page, in this case: http://rawgit.com/anmoljagetia/Visualization/iconClick/demos/dermatology.html?src/common/Icon

@anmoljagetia anmoljagetia force-pushed the anmoljagetia:iconClick branch from 5e50edb to 4934b42 May 21, 2015
Add a click event to the common/Icon.

Fixes GH-384

Signed-off-by: Anmol Jagetia anmoljagetia@gmail.com
@anmoljagetia anmoljagetia force-pushed the anmoljagetia:iconClick branch from 4934b42 to 5571181 May 21, 2015
@GordonSmith
Copy link
Member

@GordonSmith GordonSmith commented May 21, 2015

Ok - looks good!

GordonSmith added a commit that referenced this pull request May 21, 2015
Formal click event for Icon
@GordonSmith GordonSmith merged commit 04a7baf into hpcc-systems:master May 21, 2015
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@anmoljagetia anmoljagetia deleted the anmoljagetia:iconClick branch May 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants