feat(signin): show loading spinner in card #19673
Conversation
|
Putting it back to draft bc I need to tweak things a bit |
Because: * we want to show the spinner in card to prevent the flashing of the loading screen between auth pages, especially with CMS customized backgrounds This commit: * creates a loading spinner card component and use it in auth flows Closes #FXA-12344 Co-authored-by: Nick Shirley <[email protected]>
MagentaManifold
force-pushed
the
FXA-12344
branch
from
58d5558 to
bf6ec60
Compare
| */ | ||
| title?: string; | ||
| children: React.ReactNode; | ||
| children?: React.ReactNode; |
There was a problem hiding this comment.
Not a big deal, but curious why this is optional is optional now? Is there a reason to have an AppLayout without children?
There was a problem hiding this comment.
We don't need children if it's in the loading state. React.ReactNode includes undefined anyway, but it doesn't allow me to use self-closing tag unless I mark children with ?
| } | ||
|
|
||
| // !recoveryCodes check should happen after checking !totp | ||
| // TODO: pass in cmsInfo when InlineRecoverySetup supports CMS |
There was a problem hiding this comment.
Do you want to do this still?
There was a problem hiding this comment.
This is left here for future reference
| <Banner type="error" content={{ localizedHeading: error }} /> | ||
| </AppLayout> | ||
| ) : ( | ||
| <AppLayout loading /> |
There was a problem hiding this comment.
Just double checking that we don't want / need to make a call to getCmsInfo here.
There was a problem hiding this comment.
I don't add cmsInfo if the non-loading state doesn't have it
dschom
left a comment
dschom
left a comment
There was a problem hiding this comment.
Changes look good to me! I didn't have time to much manual testing here, but we can get more eyes on it once it lands.
Because
This pull request
Issue that this pull request solves
Closes: FXA-12344
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Normal layout:
Split Layout:
Mobile:
Other information (Optional)
Technically pair routes need this card spinner as well, but I don't think it's worth the effort to replicate this in Backbone