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

fixes weird legend behavior #105

Closed
wants to merge 2 commits into from
Closed

fixes weird legend behavior #105

wants to merge 2 commits into from

Conversation

cnavarreteliz
Copy link
Contributor

closes #103

Description

This bug was caused inside _drawLabel function of Viz.js, because that function always returns the last element.

For to solve it, I added a flag for to return the first value in the legend.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have documented all new methods.
  • I have written tests for all new methods/functionality.
  • I have written examples for all new methods/functionality.

Copy link
Member

@davelandry davelandry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic won't always work, as the coloring of shapes is not always mapped to the highest groupBy level. For example, the user may have 3 levels of nesting, and be coloring by the second level. In that case, we would want the 2nd level to be the label (not [0]).

Here's a proposed solution: analyze the legendData array that is created in src/_drawLegend.js, and find the deepest level where all of the data points only have 1 label. Then, make the 3rd argument in _drawLabel be an optional index of the label depth requested.

@cnavarreteliz
Copy link
Contributor Author

@davelandry I've been testing the bug and d3plus-hierarchy it's coloring by the highest groupBy level (https://jsfiddle.net/6d5ako1t/), is that a bug? Or the config is incorrect?

@davelandry
Copy link
Member

Yes, by default it will use the highest groupBy, but here's an example where the user may specify a fill corresponding to a different level: https://jsfiddle.net/6d5ako1t/1/

@cnavarreteliz
Copy link
Contributor Author

@davelandry it was very difficult guessing the highest level, because sometimes we had two properties at the same depth level, so I propose a mini-correction using this._depth

Copy link
Member

@davelandry davelandry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still does not address the original issue shown here: http://jsfiddle.net/g149pjqo/3/

To reset our conversation, there are 2 bugs that need fixing:

  1. The labels should all show the correct nesting label based on where colors are assigned. For example, the default logic (this._color in Viz.js) chooses the highest grouping level (this._groupBy[0]). The solution will probably be related to either using the this._color function, or modifying it in some way (along with modifying the legendLabel function in _drawLegend.js).
  2. When multiple groups get assigned the same color (because there are only a few default colors available), the legend should show all of their names separated by commas (currently it shows no label).

This all should be achievable without modifying the drawLabel function.

@davelandry davelandry closed this Jul 15, 2022
@davelandry davelandry deleted the bug-103 branch July 15, 2022 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected Legend Behavior in Treemap
2 participants