Add enabled option to legend #248
Conversation
…function to hide specific visualizations
| setVisibility: function (visible) { | ||
| var node = this.layer.node(); | ||
|
|
||
| visible ? $(node).show() : $(node).hide(); |
There was a problem hiding this comment.
isn't this jquery, which is not a contour dependency? Or have we aliased $ elsewhere?
There was a problem hiding this comment.
jquery is a contour dependency. regardless, I could use pure javascript (node.style.display = 'none') or even possibly d3. IMHO, doesn't really matter, as long as it works, which the above jsfiddle proves.
There was a problem hiding this comment.
jquery is definitely not a contour dependency - only d3 and lodash are. It's only included in the jsfiddles for the domready hook, and even that may be un-necessary.
There was a problem hiding this comment.
I think we may have different definitions of what contour dependencies are. Regardless, for me, if jquery is available on the global scope, then I don't see the harm in using it.
I guess what I'm trying to say is, I'm not sure what you're trying to say.
There was a problem hiding this comment.
The problem is with your assumption that jquery is available on the global scope --- Are you saying every project which uses Contour now needs to also have jquery on the global scope? If yes, that's a dependency (and an un-necessary one).
For e.g., see the docs on http://forio.com/contour/documentation.html#quickstart for including contour on a project; that (correctly) has no mention of jquery. If someone just followed the directions on that page to include contour, your code will not work.
There was a problem hiding this comment.
This (finally) makes sense. I've replaced the jquery code with javascript in my latest commit - you can see this in action at http://jsfiddle.net/q1psy5t3/3/.
| setVisibility: function (visible) { | ||
| var node = this.layer.node(); | ||
|
|
||
| visible ? node.style.display = 'block' : node.style.display = 'none'; |
There was a problem hiding this comment.
double-checking - will the default display always be block? is there never a case when it's inline/inline-block etc?
There was a problem hiding this comment.
I believe (am fairly certain) it will always be block (since it is an svg element), but I could hunt down some proof if you'd like?
There was a problem hiding this comment.
I'm not sure display will always be block for svg elements (See circle in screenshot of contour dom for e.g.), but if you're sure it will be for this specific case that's good enough
There was a problem hiding this comment.
Ah good point.
But agreed, I think we are okay for this specific case.
| @@ -5,6 +5,7 @@ | |||
| vAlign: 'middle', | |||
| hAlign: 'right', | |||
| direction: 'vertical', | |||
| enabled: true, | |||
There was a problem hiding this comment.
is there a reason to change the default? If not i'd recommend leaving the defaults as they were -- otherwise we'd need to go through all the published examples in the gallery and make sure they don't look weird.
There was a problem hiding this comment.
This property has been added to the defaults, not changed. Otherwise you would see a red line removing the previous value of this default (something like - enabled: false).
Possible that I may be misinterpreting.
There was a problem hiding this comment.
nvm, thought contour already had a legend: { enabled property, didn't realize it was just added in this commit. ignore.
| setVisibility: function (visible) { | ||
| var node = this.layer.node(); | ||
|
|
||
| visible ? node.style.display = 'block' : node.style.display = 'none'; |
There was a problem hiding this comment.
I'm not sure display will always be block for svg elements (See circle in screenshot of contour dom for e.g.), but if you're sure it will be for this specific case that's good enough
| @@ -5,6 +5,7 @@ | |||
| vAlign: 'middle', | |||
| hAlign: 'right', | |||
| direction: 'vertical', | |||
| enabled: true, | |||
There was a problem hiding this comment.
nvm, thought contour already had a legend: { enabled property, didn't realize it was just added in this commit. ignore.
parikhshiv
dismissed
narenranjit’s stale review
Discussed all recommended changes.
|
PR ready for merge. |
This PR is for #236.
As requested in the issue, I've added an
enabledoption specific to legend, and created aVisInstanceContainer#setVisibility(<bool>)function to show/hide individual visualizations.Here is a jsfiddle that illustrates use of both these features - http://jsfiddle.net/q1psy5t3/2/