Support placeholders with arbitrary width, height and borderRadius

[kadikraman]

Thank you for the amazingly useful library 😊

I'd like to suggest adding a very generic component that can be given an arbitrary width, height, borderRadius and color. I think that will be generic enough for 99% of the use cases.
The current presets only work for square images and the border radius is not customisable.

This came out of me needing a series of rectangular placeholders with a specific border radius. Even though this can be achieved by building your own Custom Component, I was wondering if perhaps that is basic enough functionality to be included out of the box.

[coveralls]

Coverage remained the same at 100.0% when pulling e022f12 on kadikraman:master into 4e6128f on mfrachet:master.

[mfrachet]

Hey :)

Sorry for being that late, and thank you for the contrib !

I find the idea interesting and it may help people with something much configurable than the current Placeholder.Media component.

The main question I have in mind is : should we consider replacing Placeholder.Media to Placeholder.Box (which, I think, could create some misunderstanding) ? Or simply adding it as a new one ?

From what I've seen inside the PR you're proposing, the Placeholder.Box component only own 1 more prop :

<Placeholder.Box
  height={50}
  width="100%"
  borderRadius={5}
  color="teal"
/>

vs

<Placeholder.Media
  color="#0000ff"
  size={70}
  hasRadius
>

What are your thougts on that ?

And again, really thank you :D

[mfrachet]

Really thank your for this PR 👍 , it's following the project code style and it helps improving this library.

I really appreciate the initiative and the code quality !

[mfrachet]

What is your thoughts on having a square box instead of a rectangle one by default ? Like setting { width: 50, height: 50} or { width: 40, height: 40 } ?

What is the most common use case you have in mind ?

[mfrachet]

I'm having the same question than the previous one concerning default values :)

[mfrachet]

What do you think of :

the background color radius of the component

[mfrachet]

What do you think of only using radius instead of borderRadius ? It's a smaller prop name, and in my thoughts, it's self descriptive ?

[kadikraman]

Hi @mfrachet
I agree, it would be nicer to extend an existing component, rather than adding a new one. I initially tried to extend the Media component to be able to handle an arbitrary width and height. However, it gets a little bit messy, due toborderRadius being a ratio of the passed in size. I ended up with this

// media.style.js
export default ({ size = 40, hasRadius = false, color = '#efefef', width, height, borderRadius }) => ({
  height: height || size,
  width: width || size,
  borderRadius: borderRadius || (hasRadius ? size / 2 : 3),
  backgroundColor: color,
});

Which doesn't beak the existing functionality, but potentially makes the API more confusing, because it relies on the combination of props being passed in.

What do you think of the above solution? Happy to change it to that if you think it's better ☺️

[kadikraman]

Hi again @mfrachet - I've updated the PR to update the Media component instead. Let me know what you think.

[coveralls]

Coverage remained the same at 100.0% when pulling 6699ab1 on kadikraman:master into 4e6128f on mfrachet:master.

[mfrachet]

I've just seen your changes and I think you're right concerning the props combination. It makes the API unclear and could create some confusion.

Maybe we could just use both Placeholder.Media & Placeholder.Box and expect some feedbacks from the community and then adjust ?

I actually really appreciate that Placeholder.Box 😄

[kadikraman]

Third time's the charm! I've changed borderRadius -> radius and defaulted to a square component as you suggested.

[mfrachet]

Seems to be the last catch :D

[kadikraman]

Whoops!

[coveralls]

Coverage remained the same at 100.0% when pulling 8150265 on kadikraman:master into 4e6128f on mfrachet:master.

[coveralls]

Coverage remained the same at 100.0% when pulling a03fe3e on kadikraman:master into 4e6128f on mfrachet:master.

[mfrachet]

LGTM 👍 thank you again 😄

[coveralls]

Coverage remained the same at 100.0% when pulling 50b1554 on kadikraman:master into 4e6128f on mfrachet:master.

[burkeshartsis]

@mfrachet any idea when this will be released onto npm?

[mfrachet]

I'll try to release a semi-major today :-)