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

fix(waffle): use descend sortPredicate by default #1510

Merged
merged 5 commits into from
Dec 3, 2021

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Dec 1, 2021

Summary

I've added to the Waffle story the sortPredicate configuration and removed an unnecessary layer configuration.

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper documentation and/or storybook story has been added or updated
  • Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

@markov00 markov00 added :docs Anything related to documentation, API, storybook :partition Partition/PieChart/Donut/Sunburst/Treemap chart related labels Dec 1, 2021
@markov00 markov00 requested review from monfera and rshen91 December 1, 2021 10:20
@monfera
Copy link
Contributor

monfera commented Dec 1, 2021

What's the sort predicate doing? It'd be nice to lock in the effect with a new eg. URL based image test so it's caught if there's regression.

One thing I remember waffle still needed is a tooltip approach where the entire color block, rather than just a single square is highlighted on hover, though I'm not too sure about how it should go with the tooltip. Ideally the entirety of a block of a given color would be perceived by the tooltip as one unit, so the tooltip location not jump as the user moves the mouse across multiple cells in a row. Same with events.

One option for that is to treat the unison of cells of a category as one thing, but render it to make it appear cell by cell.

Another option is to, at least, virtually overlay the visuals with invisible, unrendered, larger geoms.

One challenge is that cells of a color don't necessarily form a contiguous block, they can split in a way to form two disjunct areas. For example, consider the O:

XXXXXXXXXX
XXXXXXXXXX
XXXXXXXXXX
XXXXXXXXXX
XXXXXXXXXX
XXXXXXXXXX
XXXXXXXXXX
XXXXXXXXXX
XXXXXXXXOO
OOOOOYYYYY

Sometimes it's contiguous but still a rectilinear hexagon/octogon

@markov00
Copy link
Member Author

markov00 commented Dec 2, 2021

@monfera you are right, I will add a knob for that so we can control and test this.

I've found anyway something that we can probably fix on the current implementation, but please tell me if that was wanted:
in the legend.ts file you put the comment: // waffle has inherent top to bottom descending order applying a descending order to the legend items.
But the same is not applied for the data itself as for other partition charts. If I randomize the data order then I can achieve this:
Screenshot 2021-12-02 at 11 01 40
I think the we are missing the right sorting option for the waffle in the sorter used to create the partitionTree in hierarchy_of_arrays.ts

const sorter = (layout: PartitionLayout) => ({ sortPredicate }: Layer, i: number) =>
  sortPredicate ||
  (isTreemap(layout) || isSunburst(layout)
    ? descendingValueNodes
    : isMosaic(layout)
    ? i === 2
      ? ascendingValueNodes
      : descendingValueNodes
    : null);

Applying the descendingValueNodes sortPredicate by default with isWaffle(layout) should probably be desired. What do you think?

@monfera
Copy link
Contributor

monfera commented Dec 2, 2021

Applying the descendingValueNodes sortPredicate by default with isWaffle(layout) should probably be desired. What do you think?

@markov00 yes as we discussed it's the sensible approach

@markov00 markov00 changed the title docs(waffle): use sortPredicate and remove one layer fix(waffle): use descend sortPredicate by default Dec 3, 2021
@markov00 markov00 merged commit 763e2e3 into elastic:master Dec 3, 2021
@markov00 markov00 deleted the 2021_12_01-fix_waffle_story branch December 3, 2021 17:17
nickofthyme pushed a commit that referenced this pull request Dec 9, 2021
# [40.2.0](v40.1.0...v40.2.0) (2021-12-09)

### Bug Fixes

* **partition:** linkLabel textColor override ([#1498](#1498)) ([3013310](3013310))
* **waffle:** use descend sortPredicate by default ([#1510](#1510)) ([763e2e3](763e2e3))
* **xy:** stacked polarity ([#1502](#1502)) ([920666a](920666a)), closes [#1280](#1280)

### Features

* **xy:** expose style for interpolation fit functions ([#1505](#1505)) ([3071457](3071457))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:docs Anything related to documentation, API, storybook :partition Partition/PieChart/Donut/Sunburst/Treemap chart related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants