Merge pull request #4980 from ampproject/feature/4974-remove-close-on…

…-finish-screen

Update wizard UI and improve e2e tests

assets/src/setup/__mocks__/amp-setup.js

@@ -1,6 +1,10 @@
 module.exports = {
 	APP_ROOT_ID: 'amp-setup',
-	EXIT_LINK: 'http://site.test/wp-admin/?page=amp-options',
+	CLOSE_LINK: 'http://site.test/wp-admin/?page=amp-options',
+	CURRENT_THEME: {
+		name: 'Twenty Twenty',
+	},
+	FINISH_LINK: 'http://site.test/wp-admin/?page=amp-options',
 	OPTIONS_REST_ENDPOINT: 'http://site.test/wp-json/amp/v1/options',
 	READER_THEMES_REST_ENDPOINT: 'http://site.test/wp-json/amp/v1/reader-themes',
 	UPDATES_NONCE: '',

assets/src/setup/components/error-screen/index.js

@@ -19,9 +19,9 @@ import './style.css';
  *
  * @param {Object} props Component props.
  * @param {Object} props.error Error object containing a message string.
- * @param {string} props.exitLink The link to return to the admin.
+ * @param {string} props.finishLink The link to return to the admin.
  */
-export function ErrorScreen( { error, exitLink } ) {
+export function ErrorScreen( { error, finishLink } ) {
 	return (
 		<div className="error-screen-container">
 			<Panel className="error-screen">
@@ -34,7 +34,7 @@ export function ErrorScreen( { error, exitLink } ) {
 						dangerouslySetInnerHTML={ { __html: error.message || __( 'There was an error loading the setup wizard.', 'amp' ) } }
 					/>
 					{ ' ' }
-					<a href={ exitLink }>
+					<a href={ finishLink }>
 						{ __( 'Return to AMP settings.', 'amp' ) }
 					</a>
 				</p>
@@ -47,5 +47,5 @@ ErrorScreen.propTypes = {
 	error: PropTypes.shape( {
 		message: PropTypes.string,
 	} ).isRequired,
-	exitLink: PropTypes.string.isRequired,
+	finishLink: PropTypes.string.isRequired,
 };

assets/src/setup/components/error-screen/test/index.js

@@ -12,7 +12,7 @@ import { ErrorScreen } from '..';
 describe( 'ErrorScreen', () => {
 	it( 'matches snapshot', () => {
 		const wrapper = create(
-			<ErrorScreen exitLink={ 'http://my-exit-link.com' } error={ { message: 'The application failed' } } />,
+			<ErrorScreen finishLink={ 'http://my-exit-link.com' } error={ { message: 'The application failed' } } />,
 		);
 		expect( wrapper.toJSON() ).toMatchSnapshot();
 	} );

assets/src/setup/components/nav/index.js

@@ -22,9 +22,10 @@ import { User } from '../user-context-provider';
  * Navigation component.
  *
  * @param {Object} props Component props.
- * @param {string} props.exitLink Link to exit the application.
+ * @param {string} props.closeLink Link to return to previous user location.
+ * @param {string} props.finishLink Link to exit the application.
  */
-export function Nav( { exitLink } ) {
+export function Nav( { closeLink, finishLink } ) {
 	const { activePageIndex, canGoForward, isLastPage, moveBack, moveForward } = useContext( Navigation );
 	const { savingOptions } = useContext( Options );
 	const { savingDeveloperToolsOption } = useContext( User );
@@ -33,18 +34,20 @@ export function Nav( { exitLink } ) {
 		<div className="amp-setup-nav">
 			<div className="amp-setup-nav__inner">
 				<div className="amp-setup-nav__close">
-					<Button isLink href={ exitLink }>
-						<svg width="25" height="25" viewBox="0 0 25 25" fill="none" xmlns="http://www.w3.org/2000/svg">
-							<mask id="close-icon" mask-type="alpha" maskUnits="userSpaceOnUse" x="3" y="3" width="19" height="19">
-								<path fillRule="evenodd" clipRule="evenodd" d="M19.895 3.71875H5.89502C4.79502 3.71875 3.89502 4.61875 3.89502 5.71875V19.7188C3.89502 20.8188 4.79502 21.7188 5.89502 21.7188H19.895C21.005 21.7188 21.895 20.8188 21.895 19.7188V15.7188H19.895V19.7188H5.89502V5.71875H19.895V9.71875H21.895V5.71875C21.895 4.61875 21.005 3.71875 19.895 3.71875ZM13.395 17.7188L14.805 16.3088L12.225 13.7188H21.895V11.7188H12.225L14.805 9.12875L13.395 7.71875L8.39502 12.7188L13.395 17.7188Z" fill="white" />
-							</mask>
-							<g mask="url(#close-icon)">
-								<rect width="24" height="24" transform="matrix(-1 0 0 1 24.895 0.71875)" fill="#2459E7" />
-							</g>
-						</svg>
+					{ ! isLastPage && (
+						<Button isLink href={ closeLink }>
+							<svg width="25" height="25" viewBox="0 0 25 25" fill="none" xmlns="http://www.w3.org/2000/svg">
+								<mask id="close-icon" mask-type="alpha" maskUnits="userSpaceOnUse" x="3" y="3" width="19" height="19">
+									<path fillRule="evenodd" clipRule="evenodd" d="M19.895 3.71875H5.89502C4.79502 3.71875 3.89502 4.61875 3.89502 5.71875V19.7188C3.89502 20.8188 4.79502 21.7188 5.89502 21.7188H19.895C21.005 21.7188 21.895 20.8188 21.895 19.7188V15.7188H19.895V19.7188H5.89502V5.71875H19.895V9.71875H21.895V5.71875C21.895 4.61875 21.005 3.71875 19.895 3.71875ZM13.395 17.7188L14.805 16.3088L12.225 13.7188H21.895V11.7188H12.225L14.805 9.12875L13.395 7.71875L8.39502 12.7188L13.395 17.7188Z" fill="white" />
+								</mask>
+								<g mask="url(#close-icon)">
+									<rect width="24" height="24" transform="matrix(-1 0 0 1 24.895 0.71875)" fill="#2459E7" />
+								</g>
+							</svg>
 
-						{ __( 'Close', 'amp' ) }
-					</Button>
+							{ __( 'Close', 'amp' ) }
+						</Button>
+					) }
 				</div>
 				<div className="amp-setup-nav__prev-next">
 					{ 1 > activePageIndex
@@ -69,7 +72,7 @@ export function Nav( { exitLink } ) {
 
 					<Button
 						disabled={ ! canGoForward || savingOptions || savingDeveloperToolsOption }
-						href={ isLastPage && ! savingDeveloperToolsOption && ! savingOptions ? exitLink : undefined }
+						href={ isLastPage && ! savingDeveloperToolsOption && ! savingOptions ? finishLink : undefined }
 						isPrimary
 						onClick={ moveForward }
 					>
@@ -90,5 +93,6 @@ export function Nav( { exitLink } ) {
 }
 
 Nav.propTypes = {
-	exitLink: PropTypes.string.isRequired,
+	closeLink: PropTypes.string.isRequired,
+	finishLink: PropTypes.string.isRequired,
 };

assets/src/setup/components/nav/test/index.js

@@ -1,9 +1,9 @@
-
 /**
  * External dependencies
  */
 import { act } from 'react-dom/test-utils';
 import { create } from 'react-test-renderer';
+import PropTypes from 'prop-types';
 
 /**
  * WordPress dependencies
@@ -29,7 +29,24 @@ const getNavButtons = ( containerElement ) => ( {
 } );
 
 const MyPageComponent = () => <div />;
-const testPages = [ { PageComponent: MyPageComponent, slug: 'slug', title: 'Page 0' }, { PageComponent: MyPageComponent, slug: 'slug-2', title: 'Page 1' } ];
+const testPages = [
+	{ PageComponent: MyPageComponent, slug: 'slug', title: 'Page 0' },
+	{ PageComponent: MyPageComponent, slug: 'slug-2', title: 'Page 1' },
+];
+
+const Providers = ( { children, pages } ) => (
+	<OptionsContextProvider>
+		<UserContextProvider>
+			<NavigationContextProvider pages={ pages }>
+				{ children }
+			</NavigationContextProvider>
+		</UserContextProvider>
+	</OptionsContextProvider>
+);
+Providers.propTypes = {
+	children: PropTypes.any,
+	pages: PropTypes.array,
+};
 
 describe( 'Nav', () => {
 	beforeEach( () => {
@@ -44,27 +61,19 @@ describe( 'Nav', () => {
 
 	it( 'matches snapshot', () => {
 		const wrapper = create(
-			<OptionsContextProvider>
-				<UserContextProvider>
-					<NavigationContextProvider pages={ testPages }>
-						<Nav exitLink="http://site.test" />
-					</NavigationContextProvider>
-				</UserContextProvider>
-			</OptionsContextProvider>,
+			<Providers pages={ testPages }>
+				<Nav closeLink="http://site.test/wp-admin" finishLink="http://site.test" />
+			</Providers>,
 		);
 		expect( wrapper.toJSON() ).toMatchSnapshot();
 	} );
 
 	it( 'hides previous button on first page', () => {
 		act( () => {
 			render(
-				<OptionsContextProvider>
-					<UserContextProvider>
-						<NavigationContextProvider pages={ testPages }>
-							<Nav exitLink="http://site.test" />
-						</NavigationContextProvider>
-					</UserContextProvider>
-				</OptionsContextProvider>,
+				<Providers pages={ testPages }>
+					<Nav closeLink="http://site.test/wp-admin" finishLink="http://site.test" />
+				</Providers>,
 				container,
 			);
 		} );
@@ -74,4 +83,48 @@ describe( 'Nav', () => {
 		expect( prevButton ).toBeNull();
 		expect( nextButton ).not.toBeNull();
 	} );
+
+	it( 'changes next button to "Finish" on last page', () => {
+		act( () => {
+			render(
+				<Providers pages={ testPages }>
+					<Nav closeLink="http://site.test/wp-admin" finishLink="http://site.test" />
+				</Providers>,
+				container,
+			);
+		} );
+
+		const { nextButton } = getNavButtons( container );
+
+		expect( nextButton.textContent ).toBe( 'Next' );
+
+		act( () => {
+			nextButton.dispatchEvent( new global.MouseEvent( 'click', { bubbles: true } ) );
+		} );
+
+		expect( nextButton.textContent ).toBe( 'Finish' );
+	} );
+
+	it( 'close button hides on last page', () => {
+		act( () => {
+			render(
+				<Providers pages={ testPages }>
+					<Nav closeLink="http://site.test/wp-admin" finishLink="http://site.test" />
+				</Providers>,
+				container,
+			);
+		} );
+
+		const { nextButton } = getNavButtons( container );
+		let closeButton = container.querySelector( '.amp-setup-nav__close a' );
+
+		expect( closeButton ).not.toBeNull();
+
+		act( () => {
+			nextButton.dispatchEvent( new global.MouseEvent( 'click', { bubbles: true } ) );
+		} );
+
+		closeButton = container.querySelector( '.amp-setup-nav__close a' );
+		expect( closeButton ).toBeNull();
+	} );
 } );

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

@@ -37,7 +37,7 @@ export function OptionsContextProvider( { children, optionsRestEndpoint } ) {
 	const [ fetchingOptions, setFetchingOptions ] = useState( false );
 	const [ savingOptions, setSavingOptions ] = useState( false );
 	const [ didSaveOptions, setDidSaveOptions ] = useState( false );
-	const [ originalOptions, setOriginalOptions ] = useState( null );
+	const [ originalOptions, setOriginalOptions ] = useState( {} );
 
 	const { setError } = useError();
 
@@ -47,6 +47,42 @@ export function OptionsContextProvider( { children, optionsRestEndpoint } ) {
 		hasUnmounted.current = true;
 	}, [] );
 
+	/**
+	 * Fetches options.
+	 */
+	useEffect( () => {
+		if ( Object.keys( originalOptions ).length || fetchingOptions ) {
+			return;
+		}
+
+		/**
+		 * Fetches plugin options from the REST endpoint.
+		 */
+		( async () => {
+			setFetchingOptions( true );
+
+			try {
+				const fetchedOptions = await apiFetch( { url: optionsRestEndpoint } );
+
+				if ( true === hasUnmounted.current ) {
+					return;
+				}
+
+				if ( fetchedOptions.wizard_completed === false ) {
+					fetchedOptions.mobile_redirect = true;
+					fetchedOptions.reader_theme = null;
+				}
+
+				setOriginalOptions( fetchedOptions );
+			} catch ( e ) {
+				setError( e );
+				return;
+			}
+
+			setFetchingOptions( false );
+		} )();
+	}, [ fetchingOptions, originalOptions, optionsRestEndpoint, setError ] );
+
 	/**
 	 * Sends options to the REST endpoint to be saved.
 	 *
@@ -64,6 +100,16 @@ export function OptionsContextProvider( { children, optionsRestEndpoint } ) {
 				delete updatesToSave.reader_theme;
 			}
 
+			// If this is the first time running the wizard and mobile_redirect is not in updates, set mobile_redirect to true.
+			// We do this here instead of in the fetch effect to prevent the exit confirmation before the user has interacted.
+			if ( ! originalOptions.wizard_completed && ! ( 'mobile_redirect' in updatesToSave ) ) {
+				updatesToSave.mobile_redirect = originalOptions.mobile_redirect;
+			}
+
+			if ( ! originalOptions.wizard_completed ) {
+				updatesToSave.wizard_completed = true;
+			}
+
 			// Ensure this promise lasts at least a second so that the "Saving Options" load screen is
 			// visible long enough for the user to see it is happening.
 			const [ savedOptions ] = await Promise.all(
@@ -72,7 +118,7 @@ export function OptionsContextProvider( { children, optionsRestEndpoint } ) {
 						{
 							method: 'post',
 							url: optionsRestEndpoint,
-							data: { ...updates, wizard_completed: true },
+							data: updatesToSave,
 						},
 					),
 					waitASecond(),
@@ -91,7 +137,7 @@ export function OptionsContextProvider( { children, optionsRestEndpoint } ) {
 
 		setDidSaveOptions( true );
 		setSavingOptions( false );
-	}, [ optionsRestEndpoint, setError, updates ] );
+	}, [ optionsRestEndpoint, setError, originalOptions, updates ] );
 
 	/**
 	 * Updates options in state.
@@ -103,45 +149,6 @@ export function OptionsContextProvider( { children, optionsRestEndpoint } ) {
 		setDidSaveOptions( false );
 	};
 
-	useEffect( () => {
-		if ( Object.keys( updates ).length || fetchingOptions ) {
-			return;
-		}
-
-		/**
-		 * Fetches plugin options from the REST endpoint.
-		 */
-		( async () => {
-			setFetchingOptions( true );
-
-			try {
-				const fetchedOptions = await apiFetch( { url: optionsRestEndpoint } );
-
-				if ( true === hasUnmounted.current ) {
-					return;
-				}
-
-				setOriginalOptions( fetchedOptions );
-
-				const initialUpdates = {
-					wizard_completed: true,
-				};
-
-				if ( fetchedOptions.wizard_completed === false ) {
-					initialUpdates.mobile_redirect = true;
-					initialUpdates.reader_theme = null;
-				}
-
-				setUpdates( initialUpdates );
-			} catch ( e ) {
-				setError( e );
-				return;
-			}
-
-			setFetchingOptions( false );
-		} )();
-	}, [ fetchingOptions, updates, optionsRestEndpoint, setError ] );
-
 	// Allows an item in the updates object to be removed.
 	const unsetOption = useCallback( ( option ) => {
 		const newOptions = { ...updates };