Setup wizard: Done screen #4954

johnwatkins0 · 2020-06-29T23:07:13Z

Summary

This adds the final screen for the setup wizard. Uses are presented with an iframe showing an AMP page on their site in a mobile viewport, along with links to view the site or customize (in Reader mode). The "Next" button becomes "Finish" and links back to AMP settings.

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

…screen

westonruter · 2020-06-29T23:20:39Z

assets/src/setup/pages/save/index.js

+				<Phone>
+					<iframe
+						className="done__preview-iframe"
+						sandbox="allow-scripts"


Suggested change

sandbox="allow-scripts"

sandbox="allow-scripts allow-forms allow-popups allow-presentation"

You got this in just ahead of my comment below. I'll try it in a little while.

johnwatkins0 · 2020-06-29T23:21:50Z

@westonruter This PR is still WIP, but I wanted to flag for you that I made an attempt to hide the admin bar in the iframe using the sandbox attribute. I expected it to work, but for some reason it didn't. I could sense that figuring this out was potentially going to take a signicant amount of time, so I went to the fallback of setting a amp-no-admin-bar URL var and turning off the admin bar when it is set.

This PR still has the iframe with the sandbox parameter. To replicate, remove the show_admin_bar filter I added in includes/amp-helper-functions.php. Then, in the wizard, select standard mode and navigate to the last screen, which has the iframe. Where we would expect the iframe to be hidden, it's still showing.

westonruter · 2020-06-29T23:37:27Z

I may have been mistaken on the behavior of sandbox and whether it blocks cookies from being used then. Maybe it's just a matter of whether the iframed window can read cookies from the parent window or not.

westonruter · 2020-06-30T00:05:44Z

I have another idea: provide a name to the iframe, like amp-wizard-completion-preview. Then, you could do something like this:

add_action( 'wp_print_footer_scripts', function() {
	if ( ! amp_is_dev_mode() || ! is_admin_bar_showing() ) {
		return;
	}
	?>
	<script data-ampdevmode>
        document.addEventListener( 'DOMContentLoaded', () => {
            if ( document.body.classList.contains( 'admin-bar' ) ) {
                document.body.classList.remove( 'admin-bar' );
                document.documentElement.style.cssText += '; margin-top: 0 !important';
                document.getElementById( 'wpadminbar' ).remove();
            }
        } );
	</script>
	<?php
} );

But that would still result in a flash of the admin bar appearing momentarily.

In any case, this is probably overkill. If the admin bar shows up upon navigation, that's not the end of the world 😄

johnwatkins0 · 2020-06-30T01:52:10Z

This is very similar:

/**
 * Hide admin bar if the window is inside the setup wizard iframe.
 */
add_action(
	'wp_print_footer_scripts',
	function() {
		if ( ! amp_is_dev_mode() || ! is_admin_bar_showing() ) {
			return;
		}
		?>
		<script data-ampdevmode>
			document.addEventListener( 'DOMContentLoaded', function() {
				if (!('name' in window) || 'amp-wizard-completion-preview' !== window.name) { // Detects whether the current window is in an iframe.
					return;
				}

				if ( document.body.classList.contains( 'admin-bar' ) ) {
					document.body.classList.remove( 'admin-bar' );
					document.documentElement.style.cssText += '; margin-top: 0 !important';
					document.getElementById( 'wpadminbar' ).remove();
				}
			});
		</script>
		<?php
	}
);

Apparently if a window is in an iframe, the iframe's name attribute is available as window.name. I've never come across this piece of information before (that I can recall).

I don't think this is overkill because it's no more complicated and is maybe more reliable than the query var, and works across navigations, so I'm going to add it.

johnwatkins0 · 2020-06-30T04:15:58Z

Opening this for review after making a round of CSS updates not necessarily directly related to this task. I also removed the feature flag around the setup wizard, so anyone checking out this branch or develop after this branch is merged will have access to the setup wizard. No need for the AMP_NEW_ONBOARDING constant. This can easily be reversed if I've done it too soon.

github-actions · 2020-06-30T04:17:08Z

Plugin builds for c73edb0 are ready 🛎️!

Download development build
Download production build

johnwatkins0 · 2020-06-30T04:17:57Z

@jwold The production build in the above comment should work in your local environment out of the box without having to mess with PHP constants.

…screen

jwold · 2020-06-30T14:20:28Z

Can we make all the steps blue checkmarks as they get completed? https://d.pr/i/rDAmSj
I'm now wondering if we should have the gray outline: https://d.pr/i/ZMfD7t. It is feeling like a button. Curious for your thoughts! cc @amedina and @westonruter

johnwatkins0 · 2020-06-30T17:57:25Z

Can we make all the steps blue checkmarks as they get completed? https://d.pr/i/rDAmSj

I'm now wondering if we should have the gray outline: https://d.pr/i/ZMfD7t. It is feeling like a button. Curious for your thoughts! cc @amedina and @westonruter

Now being handled in #4962

johnwatkins0 · 2020-07-01T23:45:34Z

@westonruter @pierlon This is ready for review again. This PR has become a catch-all for everything we've discussed for the last couple days, and this afternoon while waiting for builds to run I refactored the e2e tests (to allow us to be able to sequence the tests -- which helps with testing persistent options). Sorry about the extra diff. Let me know if you see any issues.

westonruter · 2020-07-02T00:11:00Z

@johnwatkins0 I went through the wizard after having saved previously with a Reader theme.

The technical question is pre-selected to be non-technical. ✅
The Reader mode is selected by default. ✅
Clicking next (without changing the mode) then takes me immediately to the Summary screen. I should have been able to go to the theme selection. ❌
The mobile theme screenshot is not populated. ❌
Clicking Previous takes me to Theme selection but the screen is empty. ❌

To work around this issue I have to make a change to the mode and then switch back to Reader, and then I am taken to the theme selection and see the screenshot as expected on the summary screen.

johnwatkins0 · 2020-07-02T00:22:40Z

Thanks, @westonruter . 7ab0495 should fix that. I had to make some changes to the way options are held in the application state, so that now we have originalOptions (what's received from the endpoint), updates (changed values) and editedOptions ( { ...originalOptions, ...updates } ), and it looks I missed switching updates to editedOptions in a couple places where the latter should be used.

westonruter · 2020-07-02T00:27:31Z

@johnwatkins0 When clicking the Finish button I'm getting a JS error:

westonruter · 2020-07-02T00:28:52Z

activePageIndex is 5 so and there is no activePageIndex + 1 page:

…ect/amp-wp into feature/4707-done-screen

johnwatkins0 · 2020-07-02T00:31:35Z

@johnwatkins0 When clicking the Finish button I'm getting a JS error:

a56bd97 should fix that.

westonruter · 2020-07-02T00:38:54Z

assets/src/setup/pages/save/index.js

+					className="done__preview-iframe"
+					src={ previewPermalink }
+					title={ __( 'Site preview', 'amp' ) }
+					name="amp-wizard-completion-preview"


Minor suggestion that I'm not totally sold on myself: since the admin bar gets hidden at DOMContentLoaded there is a flash of the admin bar. This could be avoided if we hide the iframe until the page is loaded.

Suggested change

name="amp-wizard-completion-preview"

name="amp-wizard-completion-preview"

style={ { opacity: 0 } }

onLoad={ ( event ) => { event.target.style.opacity = '1'; } }

There's probably a more elegant way to do this however. (Including showing a spinner initially.)

Not important.

Done in 9b707af

westonruter · 2020-07-02T00:51:08Z

@johnwatkins0 When on the Done screen, the Close button and the Finish button do the same thing:

Should the close button be removed on the Done screen then? There's no sense to “Exit” the wizard when on the Done screen as the icon implies.

Also, I think the Close button should intentionally do history.back() to take the user to the screen they were just on (or else pass the return URL via a return query param similar to how the Customizer implements the Close button there).

Update: Filed as #4974.

westonruter · 2020-07-02T00:54:09Z

assets/src/setup/components/navigation-context-provider.js

-	// Initialize page from URL `amp-setup-screen` parameter. If not set, current page is 0.
-	// This is primarily for testing.
-	const [ activePageIndex, setActivePageIndex ] = useState( () => {
-		const index = pages.findIndex( ( { slug } ) => slug === getQueryArg( global.location.href, 'amp-setup-screen' ) );


Aside: I miss this query var 😄 It was useful to be on a screen and reload after changes to avoid having to tap Next Next Next.

Should I put it back? I thought I was probably the only person using it.

Yes, please do. It's nice for development, no?

pierlon · 2020-07-02T00:55:06Z

Should the text within the information panel on the template mode screen be selectable?

johnwatkins0 · 2020-07-02T01:16:57Z

Should the text within the information panel on the template mode screen be selectable?

Updated in 8c320a6. I think we can just let these be button-like because they will be genuine buttons in a few weeks.

assets/src/setup/pages/summary/test/redirect-toggle.js

westonruter

Functionality checks out. I'll merge once @pierlon gives his +1 on technical specifics.

pierlon

Looks good, ship it!

schlessera · 2020-07-02T15:34:52Z

schlessera · 2020-07-02T15:35:55Z

This may be because you still have "classic" set as the reader theme, which won't be accepted by the validate_callback now that we've switched that slug to legacy.

Won't other folks have this as well? We'll probably need to ensure whatever value is found is handled gracefully (by silently setting to the correct default value, for example?).

pierlon · 2020-07-02T15:41:09Z

@schlessera I think he meant the reader_theme option, which is not in production as yet.

johnwatkins0 added 5 commits June 29, 2020 11:54

Add done screen copy

d42f68f

Done screen styles

60e1a4c

Merge remote-tracking branch 'origin/develop' into feature/4707-done-…

e8978c3

…screen

Setup wizard iframe src/links

3168ead

Fix amp setup nav z-index

b035418

johnwatkins0 self-assigned this Jun 29, 2020

googlebot added the cla: yes Signed the Google CLA label Jun 29, 2020

johnwatkins0 added this to the v1.6 milestone Jun 29, 2020

westonruter reviewed Jun 29, 2020

View reviewed changes

johnwatkins0 added 5 commits June 29, 2020 22:07

CSS polish and remove new onboarding feature flag

307a02d

Testing and linting

bfc1257

Set max with on nav bar

ec5b6ce

Fix a failing PHPUnit test

9fbdd0a

Fix failing unit test in WP 4.9

ae5c868

johnwatkins0 marked this pull request as ready for review June 30, 2020 04:13

johnwatkins0 requested review from westonruter, schlessera and pierlon June 30, 2020 04:16

johnwatkins0 requested a review from jwold June 30, 2020 04:18

Merge remote-tracking branch 'origin/develop' into feature/4707-done-…

642d604

…screen

Ensure SVG clip paths have unique IDs

241c7cc

Pull current reader theme from instead of

7ab0495

Fix hiding the admin bar in legacy reader mode

de8da95

johnwatkins0 added 2 commits July 1, 2020 19:30

Skip 'next' button callback on last page when button is a link

a56bd97

Merge branch 'feature/4707-done-screen' of https://github.com/ampproj…

e882c60

…ect/amp-wp into feature/4707-done-screen

westonruter reviewed Jul 2, 2020

View reviewed changes

johnwatkins0 added 2 commits July 1, 2020 20:12

Add iframe onload handler and fix some tests

9b707af

remove pointer-events none from nonfunctioning button

8c320a6

westonruter reviewed Jul 2, 2020

View reviewed changes

assets/src/setup/pages/summary/test/redirect-toggle.js Outdated Show resolved Hide resolved

Remove unused function

c73edb0

westonruter approved these changes Jul 2, 2020

View reviewed changes

pierlon approved these changes Jul 2, 2020

View reviewed changes

westonruter merged commit 37cb245 into develop Jul 2, 2020

westonruter deleted the feature/4707-done-screen branch July 2, 2020 02:07

westonruter mentioned this pull request Jul 2, 2020

Wizard: Improve functionality of Close button vs Finish button #4974

Closed

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020

johnwatkins0 added the WS:UX Work stream for UX/Front-end label Aug 12, 2020

westonruter mentioned this pull request Apr 16, 2021

Advise users to review site after enabling AMP to ensure working as expected #6071

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup wizard: Done screen #4954

Setup wizard: Done screen #4954

johnwatkins0 commented Jun 29, 2020 •

edited

Loading

westonruter Jun 29, 2020

johnwatkins0 Jun 29, 2020

johnwatkins0 commented Jun 29, 2020

westonruter commented Jun 29, 2020

westonruter commented Jun 30, 2020

johnwatkins0 commented Jun 30, 2020 •

edited

Loading

johnwatkins0 commented Jun 30, 2020

github-actions bot commented Jun 30, 2020 •

edited

Loading

johnwatkins0 commented Jun 30, 2020

jwold commented Jun 30, 2020

johnwatkins0 commented Jun 30, 2020

johnwatkins0 commented Jul 1, 2020

westonruter commented Jul 2, 2020

johnwatkins0 commented Jul 2, 2020

westonruter commented Jul 2, 2020

westonruter commented Jul 2, 2020

johnwatkins0 commented Jul 2, 2020

westonruter Jul 2, 2020

johnwatkins0 Jul 2, 2020

westonruter commented Jul 2, 2020 •

edited

Loading

westonruter Jul 2, 2020

johnwatkins0 Jul 2, 2020

westonruter Jul 2, 2020

pierlon commented Jul 2, 2020

johnwatkins0 commented Jul 2, 2020

westonruter left a comment

pierlon left a comment

schlessera commented Jul 2, 2020

schlessera commented Jul 2, 2020

pierlon commented Jul 2, 2020

	sandbox="allow-scripts"
	sandbox="allow-scripts allow-forms allow-popups allow-presentation"

Setup wizard: Done screen #4954

Setup wizard: Done screen #4954

Conversation

johnwatkins0 commented Jun 29, 2020 • edited Loading

Summary

Checklist

westonruter Jun 29, 2020

Choose a reason for hiding this comment

johnwatkins0 Jun 29, 2020

Choose a reason for hiding this comment

johnwatkins0 commented Jun 29, 2020

westonruter commented Jun 29, 2020

westonruter commented Jun 30, 2020

johnwatkins0 commented Jun 30, 2020 • edited Loading

johnwatkins0 commented Jun 30, 2020

github-actions bot commented Jun 30, 2020 • edited Loading

johnwatkins0 commented Jun 30, 2020

jwold commented Jun 30, 2020

johnwatkins0 commented Jun 30, 2020

johnwatkins0 commented Jul 1, 2020

westonruter commented Jul 2, 2020

johnwatkins0 commented Jul 2, 2020

westonruter commented Jul 2, 2020

westonruter commented Jul 2, 2020

johnwatkins0 commented Jul 2, 2020

westonruter Jul 2, 2020

Choose a reason for hiding this comment

johnwatkins0 Jul 2, 2020

Choose a reason for hiding this comment

westonruter commented Jul 2, 2020 • edited Loading

westonruter Jul 2, 2020

Choose a reason for hiding this comment

johnwatkins0 Jul 2, 2020

Choose a reason for hiding this comment

westonruter Jul 2, 2020

Choose a reason for hiding this comment

pierlon commented Jul 2, 2020

johnwatkins0 commented Jul 2, 2020

westonruter left a comment

Choose a reason for hiding this comment

pierlon left a comment

Choose a reason for hiding this comment

schlessera commented Jul 2, 2020

schlessera commented Jul 2, 2020

pierlon commented Jul 2, 2020

johnwatkins0 commented Jun 29, 2020 •

edited

Loading

johnwatkins0 commented Jun 30, 2020 •

edited

Loading

github-actions bot commented Jun 30, 2020 •

edited

Loading

westonruter commented Jul 2, 2020 •

edited

Loading