Responsive Aspect-Ratio Sizing with padding-top css Technique

[fabb]

This PR shows how the styles on the divs of ControlledPiano and Keyboard could be adapted to responsively resize the keyboard without resorting to the DimensionsProvider workaround. It uses the padding-top css Technique.

I'm aware that this PR could be improved a little before merge, I just wanted to ask if you were open to this technique of responsive resizing. Especially functions from Keyboard.js could be extracted to be usable from ControlledPiano.js as well, as the latter needs to know and set the aspect ratio on the container div.

[fabb]

A maybe even cleaner alternative to extracting helper functions from Keyboard.js would be to move the container div from the ControlledPiano component to the Keyboard component. I‘ll try that when I get to that

[fabb]

Ok, I created a new div inside the Keyboard component instead of moving the one from ControlledPiano, because the one in the latter is needed for catching mouse events. I also adapted the readme and the demo, works great!

If you decide to merge this, the CodeSandbox also would need an update. I didn't want to fork it and reference my fork from the readme, as it's your project and this way you can more easily update the CodeSandbox in the future. Also the old CodeSandbox should still work without problems.

[kevinsqi]

@fabb I like this idea, and thank you for creating the PR and explaining things clearly! I agree it would be great to have a standard way to do aspect ratio without the react-dimensions dependency.

One note is that this change also makes it impossible to keep the existing behavior of "expand to the container's width and height" when props.width is unspecified - a behavior which I'm actually using in my own use case, so it would be necessary to re-enable that.

I don't know if this is a good idea or not, but I wonder if this approach could be used by adding a wrapper container around <Piano> instead of changing the internals. You'd basically need to recompute keyboardWidthToHeight outside the component, but I think it would work? At that point I guess it would be a wrapper component similar to DimensionsProvider, but it would be much smaller and dependency-free, and possible to bundle along with react-piano.

e.g.

const keyboardWidthToHeight = ...; // calculate this from noteRange;

<div style={{ position: 'relative', height: 0, paddingTop: `${(1 / keyboardWidthToHeight) * 100}%` }}>
  <Piano style={{ width: '100%', height: '100%', position: 'absolute', top: 0, left: 0 }} {...other props} />
</div>

(this might not actually work - just trying to think of an approach that decouples the CSS logic from the component)

I'll also want to take some time to make sure the padding-top approach works well across browsers and across different use cases. Unfortunately, I'm pretty busy these days so I might not get this merged for some time. But I do really appreciate the PR and I will try to get this in eventually!

[fabb]

One note is that this change also makes it impossible to keep the existing behavior of "expand to the container's width and height" when props.width is unspecified - a behavior which I'm actually using in my own use case, so it would be necessary to re-enable that.

True, that's basically instrinsic vs. extrinsic sizing. Maybe add an explicit prop to switch between the two?

I don't know if this is a good idea or not, but I wonder if this approach could be used by adding a wrapper container around instead of changing the internals.

I wouldn't separate the container and element, since the two need to work together to utilize the padding-top technique. A problem with your example would be that the Piano style would need to be applied to the first direct child html element, that would currently be the div of ControlledPiano. A possibly better way would be to add yet another div and put the container and element divs into an own component that wrap the Piano. div all the things 😅

const AspectRatioWrapper = (props) => {
  const keyboardWidthToHeight = ...; // calculate this from noteRange;

  return <div style={{ position: 'relative', height: 0, paddingTop: `${(1 / keyboardWidthToHeight) * 100}%` }}>
    <div style={{ position: 'absolute', top: 0, left: 0, width: '100%', height: '100%'>
      <Piano {...props} width={undefined} />
    </div>
  </div>
}

This AspectRatioWrapper component could alternatively be inserted in the render func of Piano, wrapping ControlledPiano, and only be inserted conditionally, depending on some property. Either way, the getNaturalKeyCount and getMidiNumbers funcs should be extrated into a separate file.
What do you think about this approach?

I'll also want to take some time to make sure the padding-top approach works well across browsers and across different use cases. Unfortunately, I'm pretty busy these days so I might not get this merged for some time. But I do really appreciate the PR and I will try to get this in eventually!

Sure, don't worry. I pushed my own fork to npm in the mean time: https://www.npmjs.com/package/@fabb/react-piano/v/3.2.0-pre. I hope that's ok, I'd delete it when this PR gets merged.

[fabb]

What do you think of 4e9477e? This would satisfy your requirement to be able to size the component completely from the outside, and still allows to have an aspect ratio mode, without introducing new props. The keyWidthToHeight isn't used in the case no width is provided anyways, so it's possible to use it to decide whether to let the aspect ratio sizing kick in or not. It even works when combining with width: in that case the keyboard will have the exact provided width, and still size itself in a way to satisfy the keyWidthToHeight ratio.

It's a non-backwards-compatible change though, because for clients not providing the keyWidthToHeight prop, the appearance will change when updating, they just need to add it client side, so a minor version bump should be ok I guess.

[fabb]

While this now works, I'm not convinced that it provides the best experience for a client. When not setting the keyWidthToHeight prop explicitly, and not sizing it via a container div explicitly, the height will be 0. Maybe an additional prop with the sizing mode would be better afterall?

[kevinsqi]

hey, sorry I haven't looked at this yet - I will take a look this weekend.

[fabb]

Don‘t worry, it‘s not pressing, just an offer 😉