Clean up spacing in login modal in Connect #48143
Conversation
ravicious
added
no-changelog
| * in ClusterLogin are able to span the whole width of the parent component. This way when they | ||
| * slide, they slide from one slide to another, without disappearing behind padding. | ||
| */ | ||
| export const dialogCss: StyleFunction<NoExtraProps> = props => |
There was a problem hiding this comment.
Moved this to a separate file because otherwise it breaks fast refresh in @vitejs/plugin-react-swc. Not sure how worthwhile this is, as within stories fast refresh is broken anyway as each story has a default export which is not a React component.
https://github.com/vitejs/vite-plugin-react-swc#consistent-components-exports
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
ravicious
requested review from
kimlisa
and removed request for
avatus and
kiosion
| loginAttempt: { | ||
| status: '', | ||
| statusText: '', | ||
| } as Attempt<void>, | ||
| init: () => null, | ||
| initAttempt: { | ||
| status: 'success', | ||
| statusText: '', | ||
| data: { | ||
| localAuthEnabled: true, | ||
| authProviders: [], | ||
| type: '', | ||
| hasMessageOfTheDay: false, | ||
| allowPasswordless: true, | ||
| localConnectorName: '', | ||
| authType: 'local', | ||
| } as types.AuthSettings, | ||
| } as const, |
There was a problem hiding this comment.
We can avoid these assertions, which actually hide some types issues.
| loginAttempt: { | |
| status: '', | |
| statusText: '', | |
| } as Attempt<void>, | |
| init: () => null, | |
| initAttempt: { | |
| status: 'success', | |
| statusText: '', | |
| data: { | |
| localAuthEnabled: true, | |
| authProviders: [], | |
| type: '', | |
| hasMessageOfTheDay: false, | |
| allowPasswordless: true, | |
| localConnectorName: '', | |
| authType: 'local', | |
| } as types.AuthSettings, | |
| } as const, | |
| loginAttempt: { | |
| status: '', | |
| statusText: '', | |
| data: null, | |
| }, | |
| init: () => null, | |
| initAttempt: { | |
| status: 'success', | |
| statusText: '', | |
| data: { | |
| localAuthEnabled: true, | |
| authProviders: [], | |
| hasMessageOfTheDay: false, | |
| allowPasswordless: true, | |
| localConnectorName: '', | |
| authType: 'local', | |
| }, | |
| }, |
| @@ -73,7 +81,7 @@ export default function LoginForm(props: Props) { | |||
|
|
|||
| if (!localAuthEnabled) { | |||
| return ( | |||
| <FlexBordered p={4}> | |||
| <FlexBordered px={outermostPadding}> | |||
There was a problem hiding this comment.
I'd add m={0} to remove the space at the bottom:
|
@ravicious See the table below for backport results.
|
Closes #48059.
The spacing in
ClusterConnectwas a little bit all over the place with individual elements setting their own paddings and margins. I tried to limit it as much as possible, only to discover that it's not as simple as setting a uniform padding indialogCssinClusterConnect.That's because the login modal utilizes
StepSliderwhich has individual children that slide from one side to another. In order for the animation to look seamless, those individual children need to occupy the whole width of the parent component. Otherwise they'll slide within the boundaries set by the padding of the parent, as seen in the video below.An example of how padding affects animation
In the first implementation, the whole form slides from side to side. In the other implementation, it hides behind the parent padding when going offscreen.
modal-spacing.mov
Unfortunately, a lot of our design elements set margins by themselves which is not useful when you want to set gaps uniformly with
gap. This means that despite switching a lot of places to use<Flex gap={…}>, I still had to specify things likem={0}in a lot of places.In addition, I tried to curb the spacing a little bit, this is most clearly shown when comparing
ClusterAddto the previous implementation. The idea is that if the outermost padding is set to 4, components within the dialog should not use this spacing value (or bigger) to space out elements, especially when it comes to vertical spacing. Otherwise it can lead to situations where an element is closer to a modal border than it is to another element in the modal, making those two elements appear disjointed and out of place. Spacing should be used to group elements meant to appear together.Demo
In the first tab there's a version from master, in the second tab there's the version from this branch.
cluster-connect-spacing-2.mov