Fixed issue #814 - [$100] Topcoder Member Profile: Redesign ratings graph #815
Conversation
…design ratings graph
|
@smtryingcode Can you please fix the lint errors suggested in travis-ci checks above? |
|
@vikasrohit Ohhh, sorry. Fixing this. |
|
Also, there is a bug with eslint-plugin-react which is giving linting error and that import react line is mandatory. Trying to fix that. |
|
@vikasrohit errors are fixed, and merge request is updated |
|
Thanks @smtryingcode |
|
@vikasrohit your feedback is implemented, and merge request is updated |
|
Thanks @smtryingcode. Still on of the main requirements is not fulfilled which is to make the tooltip be clickable. |
|
@smtryingcode Reviewed your submission. There are few missing things, however, overall it looks good. Please resubmit as soon as you are done with those changes. |
|
@vikasrohit Fixed issues and updated merge request. |
|
Thanks @smtryingcode |
|
@smtryingcode It is opening multiple tabs when clicking on the tooltip. Steps to reproduce:
|
|
And it seems to take two clicks to show the tooltip. |
|
@vikasrohit Fixed both the issues and updated merge request. |
| @@ -36,3 +36,5 @@ html | |||
| .fold-pusher | |||
|
|
|||
| div(ui-view="footer") | |||
|
|
|||
| #chart-tooltip | |||
There was a problem hiding this comment.
I think chart tooltip element does not makes sense in all pages, can we move it to profile page's jade file?
There was a problem hiding this comment.
No, we can't do that as we need it the end of body, to position it relatively to body. Otherwise, it will not display at correct position.
There was a problem hiding this comment.
Okay, we would revisit this after this challenge.
|
|
||
|
|
||
| d3.select('body').on('click', function(){ | ||
| if((d3.event.target.classList[0] != 'tooltip-target') && !$('#chart-tooltip .tooltip-container').hasClass('tooltip-hide') && |
There was a problem hiding this comment.
I think this logic can be simplified to a great extent by using traversing the event.target's parents. What I understood from this code is that you are trying to detected if the mouse is NOT clicked on the tooltip itself, for that, we can iterate the event.target's parents and check if we find the root class of the tooltip i.e. .tooltip-container.
There was a problem hiding this comment.
@vikasrohit I think there is no parent info for event.target in d3.js, I already tried many options. And we have to use d3.event.target to handle clicks inside the chart. Only, thing is we can optimize condition to shorten it, which I am doing.
There was a problem hiding this comment.
Gotcha. Try your best to optimize the condition so that it is more readable.
There was a problem hiding this comment.
Also, most of the conditions have different meaning other than the classes inside tooltip, so those can't be optimized more. It is because we have to handle clicks inside d3js svg also. I have done best possible as per my understanding.
There was a problem hiding this comment.
Okay. Lets proceed without this optimization thing. Will take care of this after this challenge.
|
@smtryingcode Please try to put some inline comments on the non trivial code portions. |
|
@vikasrohit fixed all, updated merge request |
|
Awesome stuff @smtryingcode. Thanks. |
|
@vikasrohit Thank you. I was a great learning for me also. |
No description provided.