feat: add Wrapper support and bounding box for dynamic width/height

[kristw]

🏆 Enhancements

1. Add bounding box for dynamic width/height calculation

When either width or height is already known, such as width="100%" height={500}.
Default <ParentSize> behavior will set the container style to {width: '100%', height: '100%'}, which may exceed the known width or height values, resulting in undesired blank space on the page. This PR add another <div> to wrap it (only when necessary). If both width and height are dynamic then no bounding box will be added.

e.g. when want 100% width and 500px height.

<div style={{width: '100%', height: 500}}>
  <ParentSize>
    {...}
  </ParentSize>
</div>

Alternative

Consider including these functionalities in the @vx/responsive rewrite.

2. Add Wrapper prop for SuperChart

Problem

Inspired by use case in dataportal.

// SuperChart with dynamic width/height
<SuperChart width="100%" height="400" chartType="line /> 

// Rough hierarchy of the code above in the DOM is like this. 
<SuperChart width="100%" height="400"> // can compute dynamic width/height
  <ChartPlugin /> // for illustration purpose only, this is abstracted by SuperChart
</SuperChart>

There is a situation when a wrapper needs to be added to fix z-index issue such as tooltip.
This won't work because the wrapper will mess with the dynamic width/height calculation.

<div style={{position: 'fixed'}}>
  // with the wrapper, now width and height become 0.
  // so the chart will disappear 😱
  <SuperChart width="100%" height="100%" chartType="line" /> 
</div>

If we want to insert something in between SuperChart and ChartPlugin to avoid messing with dynamic width/height, you either have to

A. Do not use the built-in SuperChart way of determining auto width and height. Use your own way of computing dynamic width & height, which is pretty much re-implementing a lot of logic in SuperChart.

<div style={{ width="100%", height: 400 }}>
  <ParentSize>
    // ...
    <div style={{position: 'fixed'}}>
      <SuperChart width={width} height={height} chartType="line" /> // can compute dynamic width/height
    </div>
    // ...
  </ParentSize>
</div>

B. Modify the ChartPlugin to add the wrapper. Sounds like an overkill.

Both options are undesirable extra work.

Solution

With the changes in this PR.

const Wrapper = ({children}) => (
  <div style={{position: 'fixed'}}>{children}</div>
);

return (
  <SuperChart Wrapper={Wrapper} width="100%" height="400" /> 
);

🏠 Internal

• Add unit test for one missing branch that makes the coverage ~99%. Now the chart package is 100% again.

[codecov]

Codecov Report

Merging #215 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #215      +/-   ##
==========================================
+ Coverage   99.92%   99.93%   +<.01%     
==========================================
  Files         130      157      +27     
  Lines        1406     1602     +196     
  Branches      354      419      +65     
==========================================
+ Hits         1405     1601     +196     
  Partials        1        1

Impacted Files	Coverage Δ
...es/superset-ui-chart/src/components/SuperChart.tsx	100% <100%> (ø)	⬆️
...set-ui-encodeable/src/parsers/scale/applyDomain.ts	100% <0%> (ø)
...codeable/src/parsers/createGetterFromChannelDef.ts	100% <0%> (ø)
...le/src/parsers/scale/createScaleFromScaleConfig.ts	100% <0%> (ø)
...src/parsers/scale/getScaleCategoryFromScaleType.ts	100% <0%> (ø)
...erset-ui-encodeable/src/parsers/scale/applyZero.ts	100% <0%> (ø)
...rset-ui-encodeable/src/parsers/scale/applyRound.ts	100% <0%> (ø)
...ages/superset-ui-encodeable/src/typeGuards/Base.ts	100% <0%> (ø)
...set-ui-encodeable/src/parsers/fallbackFormatter.ts	100% <0%> (ø)
...ages/superset-ui-encodeable/src/utils/isEnabled.ts	100% <0%> (ø)
... and 18 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 2793be2...e98f670. Read the comment docs.

[netlify]

Deploy preview for superset-ui ready!

Built with commit e98f670

https://deploy-preview-215--superset-ui.netlify.com

[williaster]

width and height are invalid props for React.Fragment right? Ideally we don't get propType errors in that case 🤔

[kristw]

https://stackoverflow.com/questions/48705910/does-react-warn-of-additional-props-that-a-component-wasnt-expecting

[williaster]

could this be simplified so parseLength sets value to 100% if isDynamic is true?

[williaster]

if not, below we use multiplier, why not use multiplier here instead of 100% or is this slightly different?

[kristw]

Great idea.

[williaster]

could probably consolidate to fewer lines here? I also wonder if this could be moved to the BoundingBox selector to give more context to the Fragment vs div logic?

[kristw]

Comments moved as suggested.