Add the hierarchy to legends #3553
Conversation
|
Few issues so far:
|
|
I fixed the black color issue. I hadn't realized that the legend and the actual data would not have the same labels. I added a flag to the color function to handle this case. I reduced the padding of the legends. I haven't been able to see item 2 specifically. I always see the dots as shown in item 3. |
|
Also, I added the two extra commits marked as squashable. Do you want fixup commits squashed or leave them separate? |
|
How do I restart the tests? It looks like Kibana didn't start up for some reason. |
|
I've rebased off of master to handle merge conflicts. |
| */ | ||
|
|
||
| return function (arrayOfStringsOrNumbers) { | ||
| return function (arrayOfStringsOrNumbers, matchEmptyToFirst) { |
There was a problem hiding this comment.
Why is an empty value getting passed in? It seems like thats the problem? Why would we want to return the first thing in the array if we get an empty string?
There was a problem hiding this comment.
When you have a bar chart that only has one series, the individual data points don't have a label. So it originally worked by generating a fake label just for the legend (https://github.com/elastic/kibana/blob/master/src/kibana/components/vislib/lib/data.js#L45) and creating two color functions: one for the legend (https://github.com/elastic/kibana/blob/master/src/kibana/components/vislib/lib/data.js#L53) and one for the actual data (https://github.com/elastic/kibana/blob/master/src/kibana/components/vislib/lib/data.js#L53).
So the legend has a color function that uses an array with a single string value and the data points have a color function that uses an array with a single empty string value.
I thought about adding a label to all the data points to match the legend but wasn't sure that you would want that extra work
Another thought was to modify where it grabbed the label (https://github.com/elastic/kibana/blob/master/src/kibana/components/vislib/lib/data.js#L53) and if it is blank, set it to the 'yAxisLabel'.
Would you prefer either of those to my current code? Do you have another suggestion?
|
This breaks the current behavior of expansion of truncated legend values on hover, |
|
The values seem to be sorted the same for each outer ring in the visualization, and sorted the same in the table, but differently in each bucket of the legend:
|
|
I have the legend sorted by size. That's how all my pie charts showed as. So I'll have to see how to generate a pie chart that isn't sorted and look into how to make it not change the sort. |
lodash's merge requires an object which isn't guarenteed to keep an order. So write a custom merge function that works on arrays.
Switched to a div block container and moved the text-overflow styling there.
|
I fixed the legend truncation issue and the legend sort should now match the table and viz. |
|
It looks like pull request #3641 is going to cause a lot of conflicts with my changes. If that pull request is going to go in, it would probably be better for me to restart on top of those changes. It should make it a bit easier since he is passing the entire data set into the legend. It looks like I would only need to modify the two new transform data functions and the list function in the legend class. @rashidkpc, what would you like me to do? |
|
#3641 is definitely going in, it would probably be wise to base your work on it. |
|
He @trevan I really liked where this was going. I'm going to close the issue for now, but please do reopen when you're ready. |
This fixes #3402.
It also cleaned up some unused code in legends and made the mouseover/mouseout selectors more specific.
The reason that pieNames() returns the labels in two formats is because the color() function requires an array and I also needed the hierarchy structure. So instead of creating two very similar functions, I just made pieNames() return both the data required. I did attempt to convert the hierarchy structure to an array but the order would be different which caused colors to look a little wierd.