feat: change font size on responsive for sankey and sunburst chart #977

maloun96 · 2021-02-25T12:33:47Z

Make sunburst and sankey chart responsive

BEFORE

Screen.Recording.2021-02-25.at.14.33.02.mov

AFTER

Screen.Recording.2021-03-04.at.15.16.42.mov

STORYBOOK

Screen.Recording.2021-03-05.at.14.26.48.mov

Screen.Recording.2021-03-05.at.14.24.37.mov

vercel · 2021-02-25T12:33:52Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/8h5E6QySGfU2pajt8MfGLpLUH5D9
✅ Preview: https://superset-ui-git-fork-maloun96-hotfix-responsive-dashb-149f25.vercel.app

codecov · 2021-02-25T12:44:37Z

Codecov Report

Merging #977 (a6893a9) into master (c985d94) will increase coverage by 0.06%.
The diff coverage is 22.36%.

@@            Coverage Diff             @@
##           master     #977      +/-   ##
==========================================
+ Coverage   28.04%   28.10%   +0.06%     
==========================================
  Files         408      413       +5     
  Lines        8345     8473     +128     
  Branches     1166     1198      +32     
==========================================
+ Hits         2340     2381      +41     
- Misses       5861     5939      +78     
- Partials      144      153       +9

Impacted Files	Coverage Δ
...ins/legacy-plugin-chart-sankey/src/ReactSankey.jsx	`0.00% <0.00%> (ø)`
plugins/legacy-plugin-chart-sankey/src/Sankey.js	`0.00% <0.00%> (ø)`
...s/legacy-plugin-chart-sankey/src/transformProps.js	`0.00% <ø> (ø)`
...ugins/legacy-plugin-chart-sunburst/src/Sunburst.js	`0.00% <0.00%> (ø)`
plugins/legacy-plugin-chart-sankey/src/utils.ts	`73.91% <73.91%> (ø)`
plugins/plugin-chart-table/src/buildQuery.ts	`62.50% <0.00%> (-10.58%)`	⬇️
plugins/plugin-chart-table/src/controlPanel.tsx	`34.48% <0.00%> (-3.98%)`	⬇️
...et-ui-chart-controls/src/shared-controls/index.tsx	`42.55% <0.00%> (-1.77%)`	⬇️
...ins/plugin-chart-table/src/DataTable/DataTable.tsx	`45.20% <0.00%> (-0.95%)`	⬇️
plugins/plugin-chart-table/src/transformProps.ts	`62.82% <0.00%> (-0.95%)`	⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c985d94...a6893a9. Read the comment docs.

junlincc · 2021-02-25T18:18:50Z

Thank you for the PR!
Sunburst

Have you also tested resizing the CHART in dashboard Edit mode, and resizing the WINDOW in Explore view, from different angles- vertical, horizontal and from conner, and see how they look?
did we set a fixed label text size? We prefer to have it dynamic - text size shrinks as the chart shrinks, to prevent overflow or truncation.

For Sankey,

The chart should hit its min height and width dynamically once the label text disappear. not sure how difficult it is.
Still seeing some label overlapping. We should hide label as soon as they overlap.
hovering text is out of the frame. I consider this information is more important than the chart it self, let's make sure they always show despite the size of the chart.

villebro

I think this is a great step forward for making charts adapt to their size! Let's keep iterating on these and try to establish a good pattern that we can start applying to all charts.

villebro · 2021-02-25T19:00:32Z

plugins/legacy-plugin-chart-sankey/src/Sankey.css

+}
+.dashboard .superset-legacy-chart-sankey.l text, .dashboard .superset-legacy-chart-sankey .sankey-tooltip {
+    font-size: 0.8em;
+}


nit: line change

villebro · 2021-02-25T19:02:55Z

plugins/legacy-plugin-chart-sankey/src/Sankey.js

@@ -187,7 +204,7 @@ function Sankey(element, props) {
    .attr('dy', '.35em')
    .attr('text-anchor', 'end')
    .attr('transform', null)
-    .text(d => d.name)
+    .text(d => (responsiveClass === 'xs' ? '' : d.name))


I wonder if we should have a class name for hiding the labels other than xs (no good ideas to propose right now, but thinking out loud)

maloun96 · 2021-02-26T11:31:44Z

Screen.Recording.2021-02-26.at.15.48.48.mov

@junlincc

maloun96 · 2021-03-02T09:56:51Z

Screen.Recording.2021-03-02.at.11.51.38.mov

junlincc · 2021-03-02T17:12:37Z

@maloun96 Thanks Victor, can you confirm
Resizing in Dashboard Edit mode and chart in Explore also works.
For Sankey, what does it look when you hover?

ktmud · 2021-03-03T00:22:20Z

plugins/legacy-plugin-chart-sankey/src/Sankey.js

@@ -22,6 +22,8 @@ import d3 from 'd3';
 import PropTypes from 'prop-types';
 import { sankey as d3Sankey } from 'd3-sankey';
 import { getNumberFormatter, NumberFormats, CategoricalColorNamespace } from '@superset-ui/core';
+import './Sankey.css';


Let's use styled components instead of adding new CSS files.

E.g.

superset-ui/plugins/legacy-plugin-chart-parallel-coordinates/src/ReactParallelCoordinates.jsx

Line 36 in 482da21

export default styled(ParallelCoordianes)`

Then you can pass width to the styled component to calculate a responsive font-size directly.

maloun96 · 2021-03-03T12:17:00Z

Here is demo for dashboard, edit dashboard and explore mode.

Screen.Recording.2021-03-03.at.14.24.30.mov

Screen.Recording.2021-03-03.at.14.27.45.mov

villebro

Some comments. I think this is a great step forward, but the problem main problem I feel is that we're too eagerly disabling labels here; I believe it would be very important to only disable the labels that occupy space that has already been taken. It should be possible by collecting all labels that have been determined not to overlap with previous rectangles in an array, and then check for collisions with the next element until the full list of elements have been processed. At worst it will be O(n^2), but perhaps we could put some hard limit on maximum amount of elements to be tested for overlap. Also, even with 100 elements, it would be 100*99/2=4950 iterations at maximum, which I feel should be quite manageable.

villebro · 2021-03-04T08:55:58Z

plugins/legacy-plugin-chart-sankey/src/Sankey.js

+    const areOverlapping = elementsAreOverlapping(elements);
+
+    if (!areOverlapping) {
+      elements.forEach(el => el.classList.remove('opacity-0'));
+    }


It would be nice if this only disabled the elements that occupy space that has already been taken. It would be nice if elementsAreOverlapping would return the elements that need to be visible/invisible, and those would then have their opacity changed.

plugins/legacy-plugin-chart-sankey/src/utils.ts

maloun96 · 2021-03-04T12:49:06Z

Screen.Recording.2021-03-04.at.15.16.42.mov

Check also when drag is done.
@villebro @rusackas

maloun96 · 2021-03-08T07:48:05Z

Screen.Recording.2021-03-05.at.14.26.48.mov

Screen.Recording.2021-03-05.at.14.24.37.mov

villebro

Looking really good, much better than before! One minor observation: it appears that on some collisions, both elements get wiped out despite only one really needing to be hidden (See "Land Use Change" vs "Deforestation", where "Deforestation" probably wouldn't need to be hidden). I wonder if the hiding logic could somehow be refined to take this into consideration?

maloun96 · 2021-03-08T09:46:06Z

@villebro The solution is to hide both elements if they overlap, we can adjust the code to hide only 1 element if needed but will cost performance.

villebro · 2021-03-08T09:51:09Z

@maloun96 ok, let's leave it for the backlog if this becomes a problem.

…pache-superset#977) * feat: change font size on responsive * remove logs * Sunburst * WIP * Sankey label hide * Format correct object * add styled components * Replace document with container * Return overlapping elements * Drag * Fix lint * Fix test & make tooltip absoliute based on cursor position * Update storybook for charts * Resizable in separate page * Fix responsive class for sunburst * type

feat: change font size on responsive

9bd5f82

maloun96 requested a review from a team as a code owner February 25, 2021 12:33

pull-request-size bot added the size/M label Feb 25, 2021

vercel bot deployed to Preview February 25, 2021 12:33 View deployment

remove logs

0be7bfd

vercel bot deployed to Preview February 25, 2021 12:42 View deployment

villebro reviewed Feb 25, 2021

View reviewed changes

Sunburst

d8ad749

pull-request-size bot removed the size/M label Feb 26, 2021

vercel bot deployed to Preview February 26, 2021 11:31 View deployment

pull-request-size bot added the size/L label Feb 26, 2021

WIP

aea03ab

vercel bot deployed to Preview February 26, 2021 12:54 View deployment

Sankey label hide

481b44f

vercel bot deployed to Preview March 2, 2021 09:52 View deployment

Format correct object

4cb3370

vercel bot deployed to Preview March 2, 2021 10:14 View deployment

ktmud reviewed Mar 3, 2021

View reviewed changes

add styled components

8c26555

vercel bot deployed to Preview March 3, 2021 07:48 View deployment

Replace document with container

e5b2f16

vercel bot deployed to Preview March 4, 2021 08:59 View deployment

villebro reviewed Mar 4, 2021

View reviewed changes

Return overlapping elements

cd6678a

vercel bot deployed to Preview March 4, 2021 10:00 View deployment

Drag

6711b8a

vercel bot deployed to Preview March 4, 2021 12:48 View deployment

Fix lint

d2a6029

vercel bot deployed to Preview March 4, 2021 13:00 View deployment

Fix test & make tooltip absoliute based on cursor position

7631ccc

vercel bot deployed to Preview March 4, 2021 13:16 View deployment

Update storybook for charts

39b074a

vercel bot deployed to Preview March 5, 2021 12:27 View deployment

Resizable in separate page

799e4b2

vercel bot deployed to Preview March 5, 2021 13:15 View deployment

Fix responsive class for sunburst

8f994cb

vercel bot deployed to Preview March 5, 2021 14:09 View deployment

type

a6893a9

vercel bot deployed to Preview March 5, 2021 14:22 View deployment

villebro reviewed Mar 8, 2021

View reviewed changes

villebro approved these changes Mar 8, 2021

View reviewed changes

villebro merged commit deeee7c into apache-superset:master Mar 8, 2021

villebro mentioned this pull request Mar 9, 2021

feat(native-filters): add timegrain and column filter apache/superset#13484

Merged

6 tasks

serenajiang mentioned this pull request Apr 30, 2021

Sankey tooltip is not visible on the right of the chart apache/superset#14437

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: change font size on responsive for sankey and sunburst chart #977

feat: change font size on responsive for sankey and sunburst chart #977

maloun96 commented Feb 25, 2021 •

edited

Loading

vercel bot commented Feb 25, 2021 •

edited

Loading

codecov bot commented Feb 25, 2021 •

edited

Loading

junlincc commented Feb 25, 2021 •

edited

Loading

villebro left a comment

villebro Feb 25, 2021

villebro Feb 25, 2021

maloun96 commented Feb 26, 2021 •

edited

Loading

maloun96 commented Mar 2, 2021

junlincc commented Mar 2, 2021 •

edited

Loading

ktmud Mar 3, 2021 •

edited

Loading

maloun96 commented Mar 3, 2021 •

edited

Loading

villebro left a comment •

edited

Loading

villebro Mar 4, 2021

maloun96 commented Mar 4, 2021 •

edited

Loading

maloun96 commented Mar 8, 2021

villebro left a comment

maloun96 commented Mar 8, 2021

villebro commented Mar 8, 2021

feat: change font size on responsive for sankey and sunburst chart #977

feat: change font size on responsive for sankey and sunburst chart #977

Conversation

maloun96 commented Feb 25, 2021 • edited Loading

BEFORE

AFTER

vercel bot commented Feb 25, 2021 • edited Loading

codecov bot commented Feb 25, 2021 • edited Loading

Codecov Report

junlincc commented Feb 25, 2021 • edited Loading

villebro left a comment

Choose a reason for hiding this comment

villebro Feb 25, 2021

Choose a reason for hiding this comment

villebro Feb 25, 2021

Choose a reason for hiding this comment

maloun96 commented Feb 26, 2021 • edited Loading

maloun96 commented Mar 2, 2021

junlincc commented Mar 2, 2021 • edited Loading

ktmud Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

maloun96 commented Mar 3, 2021 • edited Loading

villebro left a comment • edited Loading

Choose a reason for hiding this comment

villebro Mar 4, 2021

Choose a reason for hiding this comment

maloun96 commented Mar 4, 2021 • edited Loading

maloun96 commented Mar 8, 2021

villebro left a comment

Choose a reason for hiding this comment

maloun96 commented Mar 8, 2021

villebro commented Mar 8, 2021

maloun96 commented Feb 25, 2021 •

edited

Loading

vercel bot commented Feb 25, 2021 •

edited

Loading

codecov bot commented Feb 25, 2021 •

edited

Loading

junlincc commented Feb 25, 2021 •

edited

Loading

maloun96 commented Feb 26, 2021 •

edited

Loading

junlincc commented Mar 2, 2021 •

edited

Loading

ktmud Mar 3, 2021 •

edited

Loading

maloun96 commented Mar 3, 2021 •

edited

Loading

villebro left a comment •

edited

Loading

maloun96 commented Mar 4, 2021 •

edited

Loading