Room UI Redesign: Safari Fixes #3319
Conversation
There was a problem hiding this comment.
Thanks for meticulously fixing up the experience for safari users! I don't have a device setup to test these changes in safari but as far as I can tell, I didn't find any regressions.
--
Unrelated to this PR but not sure where else to mention it. The items in the ButtonGridPopover require a name but I believe this is supposed to be label judging by where it's used. This causes some react prop type warnings.
Any thoughts on how to catch things like this in react? Seems like the propTypes we define are part of a solution. Does it make sense to try to explicitly disallow props that are not listed in propTypes? (Is this even possible, or would it mess up component hierarchies?)
|
|
||
| export function Column({ as: Component, className, gap, padding, center, grow, overflow, children, ...rest }) { | ||
| const gapClass = gap === true ? styles.mdGap : styles[`${gap}Gap`]; | ||
| const paddingClass = padding === true ? styles.lgPadding : styles[`${padding}Padding`]; |
There was a problem hiding this comment.
Very minor concern but it's a little surprising that the default values for gap and padding are different (mdGap and lgPadding). However I doubt this will ever cause us issues so if it feels good to write gap instead of gap="md" that's fine by me
There was a problem hiding this comment.
They just correspond to the standard spacing units I've come up with. I'll document them and move them to theme.scss
There was a problem hiding this comment.
Looks like things work really well with everything using the defaults of md and lg. Where md went from 20px to 16px and lg went from 24px to 20px. This actually fits the designs better too. Also they use the same units as the font sizes.
|
If you learned anything particularly interesting about how Safari was failing here, it might actually be worth filing a webkit bug (or bugs) with reduced test cases, once the redesign is out. Would be nice to improve the state of things for everyone, and we're just going to keep running into Safari bugs going forward if they are never reported and fixed. |
|
@brianpeiris it was mostly things around explicitly setting |
This PR implements a number of fixes for Safari and other cross browser issues. It also introduces the
Columncomponent which is used to clean up a number of repetitive styles.