[11333] Update the splash screen's layout #11492
Conversation
|
Build files for 7bbba32 have been deleted. |
|
Size Change: +104 kB (+5.18%) 🔍 Total Size: 2.12 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Hey @ankitrox, this is off to a good start, but there are a few issues to address. Please see my comments.
Additionally, the margin of the content has a grey background, whereas it should be white as per the design.
For reference, here's a screen grab of the Figma design. This shows how the background should be white rather than grey.
It looks like we'll need to add the collapsed prop to the Grid rendered in SetupUsingProxyWithSignIn, and add a class to the .googlesitekit-setup element which sets its background colour to white to accomplish this.
One more point, the screenshot is not being clipped when it's taller than the page - you can see the scrollbar in my screen grab, which again should not be present in this case.
As per the AC:
... for viewports which are shorter than the screenshot image, the intention is to have the screenshot image clipped at the bottom edge of the viewport rather than allowing it to overflow and be scrollable.
I've not dug into the fix for this last point, which I'll leave with you to investigate.
| let title; | ||
| let description; | ||
| let showLearnMoreLink = false; | ||
| let getHelpURL = null; |
There was a problem hiding this comment.
This getHelpURL variable should be restored, and passed as a prop to both LegacySplash and RefreshedSplash.
RefreshedSplash should be updated to render the "Get help" link.
| 'google-site-kit' | ||
| ); | ||
|
|
||
| getHelpURL = changedURLHelpLink; |
There was a problem hiding this comment.
This line also needs to be restored as per the above.
| const analyticsModuleActive = useSelect( ( select ) => | ||
| select( CORE_MODULES ).isModuleActive( MODULE_SLUG_ANALYTICS_4 ) | ||
| ); | ||
| const connectedProxyURL = useSelect( ( select ) => |
There was a problem hiding this comment.
There's no need to move this useSelect(), please restore it to its previous position to avoid unnecessary changes.
| const changedURLHelpLink = useSelect( ( select ) => | ||
| select( CORE_SITE ).getDocumentationLinkURL( 'url-has-changed' ) | ||
| ); |
There was a problem hiding this comment.
This definition of changedURLHelpLink will also need to be restored.
| } else { | ||
| title = __( 'Set up Site Kit', 'google-site-kit' ); | ||
| description = __( | ||
| 'Get insights on how people find your site, as well as how to improve and monetize your site’s content, directly in your WordPress dashboard', | ||
| 'google-site-kit' | ||
| ); | ||
| title = setupFlowRefreshEnabled | ||
| ? __( "Let's get started!", 'google-site-kit' ) | ||
| : __( 'Set up Site Kit', 'google-site-kit' ); | ||
| description = ! setupFlowRefreshEnabled | ||
| ? __( | ||
| 'Get insights on how people find your site, as well as how to improve and monetize your site’s content, directly in your WordPress dashboard', | ||
| 'google-site-kit' | ||
| ) | ||
| : null; | ||
| } |
There was a problem hiding this comment.
This can be simplified a bit. Also, please update "Let's get started" to use a smart apostrophe for consistency:
} else if ( setupFlowRefreshEnabled ) {
title = __( "Let’s get started!", 'google-site-kit' );
description = __(
'Get insights on how people find your site, as well as how to improve and monetize your site’s content, directly in your WordPress dashboard',
'google-site-kit'
);
} else {
title = __( 'Set up Site Kit', 'google-site-kit' );
}| { DISCONNECTED_REASON_CONNECTED_URL_MISMATCH === | ||
| disconnectedReason && | ||
| connectedProxyURL !== homeURL && ( | ||
| <P> | ||
| { sprintf( | ||
| /* translators: %s: Previous Connected Proxy URL */ | ||
| __( '— Old URL: %s', 'google-site-kit' ), | ||
| connectedProxyURL | ||
| ) } | ||
| <br /> | ||
| { sprintf( | ||
| /* translators: %s: Connected Proxy URL */ | ||
| __( '— New URL: %s', 'google-site-kit' ), | ||
| homeURL | ||
| ) } | ||
| </P> | ||
| ) } |
There was a problem hiding this comment.
Please move this block directly below the p.googlesitekit-setup__description element, as it is in the legacy splash screen.
As things stand, the Analytics checkbox is rendered above the "Old URL" / "New URL" message, whereas this message should be directly beneath the description.
| }, | ||
| }; | ||
|
|
||
| export const RefreshedSetupFlow = Template.bind( {} ); |
There was a problem hiding this comment.
Adding these stories to this file makes the menu quite unwieldy:
I'd suggest creating a new file for the stories, named index-setupFlowRefresh.stories.js, and use the title property of the default export configuration to add a folder named SetupFlowRefresh. We can then replicate the current story names, and this would result in a tidier menu:
assets/js/components/setup/SetupUsingProxyWithSignIn/LegacySplashContent.js
Show resolved
Hide resolved
|
Thank you @techanvil for reviewing the PR and adding your feedback. I've addressed the feedback except the clipping of screenshot at bottom of the viewport. As discussed over slack thread, we need to account for how we need to handle the admin footer, once we have clarity on it we can work on clipping the screenshot image at viewport limit. |
assets/js/components/setup/SetupUsingProxyWithSignIn/SplashContent.js
Outdated
Show resolved
Hide resolved
assets/js/components/setup/SetupUsingProxyWithSignIn/index.test.js
Outdated
Show resolved
Hide resolved
assets/js/components/setup/SetupUsingProxyWithSignIn/index.test.js
Outdated
Show resolved
Hide resolved
|
Thank you @techanvil for reviewing the changes. I have made the updates as suggested. |
There was a problem hiding this comment.
Thanks @ankitrox. This needs another iteration, please see my comments.
Also, I should have spotted these last time, but I've noticed a few places where the styling needs updating to match Figma.
- The checkboxes should be vertically aligned with the first line of their associated text.
- The divider line should extend to the right-hand edge of the content.
- There should not be a gap between the green background and the edge of the viewport.
Here's a screenshot of Figma for reference:
| { title } | ||
| </Typography> | ||
|
|
||
| <p className="googlesitekit-setup__description"> |
There was a problem hiding this comment.
We should not render this p tag when it's empty, as it adds unwanted space between the title and the checkbox.
| <p className="googlesitekit-setup__description"> | |
| { ( showLearnMoreLink || description ) && ( | |
| <p className="googlesitekit-setup__description"> |
There was a problem hiding this comment.
Added the condition so as not to render empty p tag.
| } ); | ||
|
|
||
| describe( 'with the `setupFlowRefresh` feature flag enabled', () => { | ||
| it( 'should render the setup page with the refreshed layout', async () => { |
There was a problem hiding this comment.
Again, let's avoid calling it "refreshed" in code.
| it( 'should render the setup page with the refreshed layout', async () => { | |
| it( 'should render the setup page correctly', async () => { |
| } ) | ||
| ); | ||
|
|
||
| const { container, queryByText, waitForRegistry } = render( |
There was a problem hiding this comment.
As a matter of convention we prefer to use getByText() when we do expect the element to exist:
| const { container, queryByText, waitForRegistry } = render( | |
| const { container, getByText, waitForRegistry } = render( |
| expect( container ).toMatchSnapshot(); | ||
| } ); | ||
|
|
||
| it( 'should render the setup page with Activate Analytics notice when the Analytics module is inactive', async () => { |
There was a problem hiding this comment.
Let's update this description for consistency with the test below:
| it( 'should render the setup page with Activate Analytics notice when the Analytics module is inactive', async () => { | |
| it( 'should render the setup page with the Analytics checkbox when the Analytics module is inactive', async () => { |
assets/js/components/setup/SetupUsingProxyWithSignIn/SplashContent.js
Outdated
Show resolved
Hide resolved
assets/js/components/setup/SetupUsingProxyWithSignIn/SetupFlowSVG.js
Outdated
Show resolved
Hide resolved
Thanks @techanvil I've addressed other points as mentioned and following as well.
|
There was a problem hiding this comment.
Thanks @ankitrox.
- For the gap between green background and right hand side of the viewport, I think this will be addressed when we address the screenshot clipping and hiding of admin footer because currently that gap is showing because of the vertical scrollbar.
From what I'm seeing, the gap isn't appearing because of the scrollbar - here's a screenshot of how it looks in a viewport with no scrollbar, the gap still appears.
However, as mentioned in one of my comments, we need to update this to show gutters around the grid, and this will resolve this gap, so it should no longer be an issue.
Thanks @techanvil I've addressed the points which includes.
|
There was a problem hiding this comment.
Thanks @ankitrox!
There are what I'm hoping are a final few issues to address after which this should be good to go. Please see my comments.
Additionally, I've noticed that at viewport widths 1281-1455px, the green background graphic is not visible at all at the right hand edge of the pane. Ideally, we should ensure there's a bit of green between the screenshot and the edge of the pane for all viewport widths, but as we haven't actually got a responsive design defined yet, I think it's OK to leave this as it is for now.
However, we should ensure the bottom right corner of the screenshot is clipped, at the moment it overflows the curved border of the container.
We can achieve this by adding the overflow: hidden rule to .googlesitekit-setup .googlesitekit-layout in _googlesitekit-setup.scss. This looks safe to me. In fact I suspect we can apply it globally to .googlesitekit-layout, but as Layout is used in quite a few places it's probably better to be cautious and constrain it to the .googlesitekit-setup class for now.
.googlesitekit-setup {
.googlesitekit-layout {
overflow: hidden;
}I've also noticed the output of the "Default - Staged environment warning" story is identical to "Default", however this is also the case for the current splash screen story so I'll create a separate issue to fix both stories and we can leave it as-is for this PR.
| analyticsModuleActive: PropTypes.bool, | ||
| analyticsModuleAvailable: PropTypes.bool, | ||
| children: PropTypes.func, | ||
| connectedProxyURL: PropTypes.string, |
There was a problem hiding this comment.
connectedProxyURL can also be a boolean, please update this prop type.
| connectedProxyURL: PropTypes.string, | |
| connectedProxyURL: PropTypes.oneOfType( [ | |
| PropTypes.string, | |
| PropTypes.bool, | |
| ] ), |
| analyticsModuleActive: PropTypes.bool, | ||
| analyticsModuleAvailable: PropTypes.bool, | ||
| children: PropTypes.func, | ||
| connectedProxyURL: PropTypes.string, |
There was a problem hiding this comment.
connectedProxyURL can also be a boolean, please update this prop type.
| connectedProxyURL: PropTypes.string, | |
| connectedProxyURL: PropTypes.oneOfType( [ | |
| PropTypes.string, | |
| PropTypes.bool, | |
| ] ), |
There was a problem hiding this comment.
Please add VRT scenarios for these stories. We'll be adding scenarios for the existing splash screen stories in #11574.
|
Thanks @techanvil I've updated the css as well as added VRT scenarios as suggested. |
There was a problem hiding this comment.
Thanks @ankitrox.
There are however still some fixes needed, and with this already significantly over the estimate I've pushed changes to address most of them myself to save time.
I've fixed the grid gutters and the Analytics checkbox alignment, simplified the screenshot rendering and removed an unnecessary version of it, and tweaked the styling for the screenshot to better align with the design.
Please review my changes to ensure you're familiar and happy with them, update the VRT reference images, and address the last remaining comment on the PR.
| import SetupFlowSVG from './SetupFlowSVG'; | ||
| import SplashGraphic from '@/svg/graphics/splash-graphic.svg'; |
There was a problem hiding this comment.
Sorry I didn't spot this before, but these names can be improved (please rename the component and filenames accordingly):
| import SetupFlowSVG from './SetupFlowSVG'; | |
| import SplashGraphic from '@/svg/graphics/splash-graphic.svg'; | |
| import SplashScreenshotSVG from './SplashScreenshotSVG'; | |
| import SplashBackground from '@/svg/graphics/splash-background.svg'; |
assets/js/components/setup/SetupUsingProxyWithSignIn/SplashContent.js
Outdated
Show resolved
Hide resolved
…ite-kit-wp into enhancement/11333-splash-screen.
|
Thanks @techanvil for committing the changes. I've made the updates as requested, also updated the VRT images. |
There was a problem hiding this comment.
Thanks @ankitrox!
This is nearly there now. There are a couple of Storybook/VRT changes needed, please see my comments.
Also, please update the QAB - you can remove the point the screenshot on wider viewports, and it would also be good to point out that the responsive behaviour is not defined yet so we don't expect the layout of the graphics to precisely match up with the Figma design.
Summary
Addresses issue:
Relevant technical choices
As per the discussion with @techanvil, the clipping of screenshot at the bottom of the viewport is not added in this PR because we need to get clarity over the admin footer where it will be visible anyways causing the vertical scrollbar to appear on the page. Once we finalize over the footer, we can add the change for fix height of the main container.
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