Faithlife / styled-ui

Faithlife UI styleguide and set of components
https://faithlife.github.io/styled-ui/
MIT License
13 stars 50 forks source link

Improve `Modal.FooterButtons` API #529

Closed TyMick closed 2 years ago

TyMick commented 2 years ago

Part of FLCOM-8106.

Implements the more flexible Modal.FooterButtons API discussed in this Styled UI Flowdock thread and deprecates the old commitButton, cancelButton, and deleteButton props in favor of it.

A summary of the API change:

024fc0993b0b8a4cadf04566d246d6a7338debbf is actually the only fix I originally needed for FLCOM-8106, but I think I'll use the new API for that case anyway, since I like it better. 🥳

Important review points

I'd recommend walking through this PR one commit at a time when reviewing. Each step is pretty atomic.

korbinancell commented 2 years ago

requiring the dev to enter it

What do you think of adding a floatLeft or alignLeft prop that applies this css?

TyMick commented 2 years ago

Maybe floatAcross (or floatOpposite) since I switched the right/left properties to logical properties?

TyMick commented 2 years ago

I've gone with floatAcross for now. (And edited past commits to do so, to keep the commits organized—sorry if that throws off anyone's review.)

TyMick commented 2 years ago

Rebasing to pull in #530 (to make it easier to yalc publish).

PatrickNausha commented 2 years ago

Was https://github.com/Faithlife/styled-ui/commit/3f8bce08ff027cc860d6271710a33b58ed14a755 a good decision? Removing the element measuring will make the component much more maintainable, and the useFullWidthButtons behavior was only triggering on screens less than 320 CSS pixels wide, so these full-width buttons wouldn't even be used on an iPhone 5/SE.

This logic seems sound to me. If useFullWidthButtons is never true in practice, the code seems effectively dead. In theory a consumer could make a modal as narrow as they want, shrinking it below 320px, but I'm not sure why they would.

TyMick commented 2 years ago

Thanks so much for the review, @PatrickNausha! Would you like to take one more pass before I merge?

PatrickNausha commented 2 years ago

I just saw the documentation page for the first time. Should Modal.FooterButton be on that page?

TyMick commented 2 years ago

Ready for another pass, @PatrickNausha; thanks again!