Skip to content
This repository has been archived by the owner on May 30, 2023. It is now read-only.

[D3: Tree Map] issue w/ test #8 #54

Closed
no-stack-dub-sack opened this issue Feb 20, 2017 · 3 comments · Fixed by #69
Closed

[D3: Tree Map] issue w/ test #8 #54

no-stack-dub-sack opened this issue Feb 20, 2017 · 3 comments · Fixed by #69

Comments

@no-stack-dub-sack
Copy link
Member

copying the issue that was raised on the forum by @leonfeng here so it can be addressed:

Original issue

This one made my head hurt 😵 https://codepen.io/leonfeng/pen/xgdqGR7

Personally I felt it was quite a leap from the D3 challenges to this treemap project. The D3 docs only helped a little. In the end I had to "borrow" code from a (GPL-licensed) tutorial, and to be honest I still don't understand half of the code.

In addition, I used a 3rd-party library to render the legend. And it fails the legend colours test (#9) even though each legend item has a unique colour. Are we expected to manually code the legend items? The user stories aren't clear on that 😕

@Christian-Paul's response:

@leonfeng Thanks for the feedback!

Test #9 states: "The legend items should use at least 2 different fill colors"

It expects the elements that have the different fill colors to have the class "legend-item"

In your project, only one element has the class of legend-item; if you made the path elements with class="swatch" also have the class of "legend-item" it should pass. We've tried to make the tests as general as possible so campers can employ whatever strategy they see fit, so using a library is fine, insofar as you can label the classes.

As for how we can improve the tests, @no-stack-dub-sack, @QuincyLarson, @paycoguy, I think making test #8 check for the presence of more than one element with class="legend-item" would make it more clear that there should be multiple elements with this class.

If popular legend libraries do not allow campers to assign class names, we'll need a different solution: perhaps we could iterate through the children of the top level legend element, and collect fill color information that way.

@no-stack-dub-sack
Copy link
Member Author

@Christian-Paul do you want to take a stab at implementing your suggestion above?

@Christian-Paul
Copy link
Contributor

@no-stack-dub-sack Sure! Which one do you think seems more suitable? Altering test 8 to check for more than one legend item, or checking all children of the legend element for fill color data? Or both?

@no-stack-dub-sack
Copy link
Member Author

@Christian-Paul Well, not sure I guess, because it seems like the variable, as you mentioned, is if libraries do not allow for campers to add class names. If we think this might be the case, it seems like the latter suggestion may be a safer bet.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants