-
Notifications
You must be signed in to change notification settings - Fork 290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhancement/9336 siwg setup page #9511
Conversation
Build files for 64c878d have been deleted. |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @zutigrm this was a good read and test, a couple of points:
-
I noticed two issues with the SVG in different responsive sizes:
- At the smaller breakpoints there is a small ~1px white gap between the graphic and the footer:
- On larger screens the SVG is cropped or cut at the bottom cutting off the drop shadow of the graphic harshly:
One other note, this SetupFrom does not implement any error handling, should we implement something like the assets/js/components/StoreErrorNotices.js
component in this setup form?
> | ||
<LazyGraphicSVG /> | ||
</MediaErrorHandler> | ||
</Suspense> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one for considering lazy loading here, that's something Evan has mentioned a few times recently and it something that should be considered in each ticket that adds new media.
@benbowler Thanks, good catch!
Updated, thanks.
site-kit-wp/assets/js/modules/sign-in-with-google/components/setup/SetupForm.js Lines 43 to 46 in dd45c14
There isn't a space there, might be visual trick due to the grey border, since since there is no spacing between border and graphic: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @zutigrm, there was a 1px gap here, I pushed a quick fix. All other points look good, moving through to MR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @zutigrm. This is a good start. Added some feedback for you. Please, take a look.
assets/js/modules/sign-in-with-google/components/setup/SetupForm.js
Outdated
Show resolved
Hide resolved
assets/js/modules/sign-in-with-google/components/setup/SetupMain.js
Outdated
Show resolved
Hide resolved
assets/js/modules/sign-in-with-google/components/setup/SetupMain.js
Outdated
Show resolved
Hide resolved
assets/js/modules/sign-in-with-google/components/setup/SetupFormSVG.js
Outdated
Show resolved
Hide resolved
...op/reference/google-site-kit_Modules_SignInWithGoogle_Setup_SetupMain_0_document_0_small.png
Outdated
Show resolved
Hide resolved
...op/reference/google-site-kit_Modules_SignInWithGoogle_Setup_SetupMain_0_document_2_large.png
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @zutigrm. Almost looks good to me. Just added two last comments and it should be good to go.
import SetupForm from './SetupForm'; | ||
import Badge from '../../../../components/Badge'; | ||
|
||
export default function SetupMain( { finishSetup = () => {} } ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can omit finishSetup
for this component because we don't need it in the form component.
export default function SetupMain( { finishSetup = () => {} } ) { | |
export default function SetupMain() { |
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist