Centerer: Remove, as unnecessary.#4868
Conversation
AccountPickScreen and AuthScreen both pass `centerContent` to the `<Screen />` that they use. Empirically, this strategy seems to do just fine on its own in both cases: removing the `<Centerer />` in both cases doesn't seem to change anything at all, in my testing. This is as we'd expect, from reading `Screen`'s interface and noting that it seems to work just fine without a `<Centerer />` in three existing uses. What's more, we have a hypothesis that the `<Centerer />` is causing an important bug that prevents some users from logging in at all, by obscuring crucial pieces of the UI; see https://chat.zulip.org/#narrow/stream/48-mobile/topic/login.20bug/near/1225893.
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks @chrisbobbe! I'll be surprised if what's in this component is affecting anything like that bug, but good to clean it up. One comment below.
| }, | ||
| inner: { | ||
| width: '100%', | ||
| maxWidth: 480, |
There was a problem hiding this comment.
This maxWidth looks like it was probably doing something. In particular, on AccountPickScreen, if your screen is wide (like on a tablet, or in landscape on a phone) then I think this is what stops the items in the accounts list from stretching across the whole width.
Maybe move this down into the two callsites? Those are better places for this layout choice to appear anyway.
There was a problem hiding this comment.
Makes sense.
Hmm, here's a question about Screen's interface:
When you pass centerContent to a Screen, that gives justifyContent: 'center' to a container element (so, centers that element's children vertically), but doesn't set alignItems: 'center', to center content horizontally; alignItems is left with its default, 'stretch'. This means children with a maxWidth aren't centered horizontally, but rather sit to the left. Children without a maxWidth are stretched to fill the container (so I guess they are effectively centered).
Would it be an improvement to Screen's interface to have centerContent set both justifyContent and alignItems to 'center'?
https://css-tricks.com/snippets/css/a-guide-to-flexbox/#align-items
There was a problem hiding this comment.
Yeah, good find -- I think that would be an improvement.
|
I'd like to pause on this and will come back once #4893 is resolved. |
We're not totally satisfied with how we annotate `defaultProps` (see f801c97), but it works, and we'd like to remove `Centerer` anyway; issue zulip#4868 is in progress toward that. Related: zulip#4907
We're not totally satisfied with how we annotate `defaultProps` (see f801c97), but it works, and we'd like to remove `Centerer` anyway; issue zulip#4868 is in progress toward that. Related: zulip#4907
|
Closing as stale, with TODOs added in b55d0b3. |
2 participants
AccountPickScreen and AuthScreen both pass
centerContentto the<Screen />that they use. Empirically, this strategy seems to dojust fine on its own in both cases: removing the
<Centerer />inboth cases doesn't seem to change anything at all, in my testing.
This is as we'd expect, from reading
Screen's interface and notingthat it seems to work just fine without a
<Centerer />in threeexisting uses.
What's more, we have a hypothesis that the
<Centerer />is causingan important bug that prevents some users from logging in at all, by
obscuring crucial pieces of the UI; see
https://chat.zulip.org/#narrow/stream/48-mobile/topic/login.20bug/near/1225893.