Gutenboarding: Domain Picker Modal #41212

yansern · 2020-04-17T07:41:23Z

Changes proposed in this Pull Request

This PR introduces Domain Picker Modal which is enabled via the gutenboarding/domain-picker-modal feature flag.

There are some architectural decisions being made here so feel free to review & discuss them in this PR.

The component tree looks like this:

DomainPickerButton
   |_DomainPickerPopover
     |_DomainPicker
   |_DomainPickerModal
     |_DomainPicker

Here are the list of changes:

DomainPickerModal component.
This new component uses the react-modal component and reuses the DomainPicker component. It reuses <html> element's scrollbar to scroll down modal's content while keeping the gutenboarding header visible. https://github.com/Automattic/wp-calypso/pull/41212/files#diff-3b763db88828368a2cd772bdcb184e7bR1-R24
DomainPickerPopover component.
This is refactored from the existing code in DomainPickerButton component.
New props showDomainConnectButton & showDomainCategories introduced in DomainPicker component.
The parent component DomainPickerPopover and DomainPickerModal decides what to show and what to hide. https://github.com/Automattic/wp-calypso/pull/41212/files#diff-9fb3f50c91d02f132bfab858e87cb792R74-R93
Close Button & More Options Button is now outside of DomainPicker component
These 2 buttons are specific to DomainPickerPopover, therefore it make sense that DomainPickerPopover component should play the role of decorating the DomainPicker component with these 2 additional buttons. https://github.com/Automattic/wp-calypso/pull/41212/files#diff-3acfaab9099bccb32e73498b4be055a4R55-R72 This also fixes the accessibility issue with close button needing a tabIndex={ -1 } to fix incorrect autofocusing. p1586489720342200-slack-gutenboarding

Other changes:

onClose event removed from CloseButton component
onClose is basically the same as onClick. It was removed to prevent typing issues as onClose did not accept undefined values. This component is only used by DomainPicker component so far.

Testing instructions

Note: You need to test this locally. Can't access the modal via the live test page as this is hiding behind a feature flag.

Go to /new and enter your site name.
Click on domain picker button.
Domain picker popover should appear.
- Due to refactoring, please test all functionalities of domain picker inside a popover, it should behave like before.
- You should now be able to cycle-tab all the form elements in the domain picker popover, including the close button.
Click on the More Options button
Domain picker modal should appear.
- Domain picker button should disappear.
- Resize the browser viewport height until a scrollbar appears.
- Try scrolling the modal content.
Try opening the domain picker modal on every other gutenboarding pages.

Screenshot

Fixes #41081

…lesheet.

…odal.

matticbot · 2020-04-17T07:41:26Z

Test live: https://calypso.live/?branch=add/gutenboarding-domain-picker-modal

matticbot · 2020-04-17T07:54:42Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~5802 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding     +22782 B  (+1.0%)    +5802 B  (+1.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

dubielzyk · 2020-04-17T08:26:36Z

We're moving to a sticky footer bar for the main buttons on mobile. Can we implement that here?

yansern · 2020-04-17T08:28:36Z

We're moving to a sticky footer bar for the main buttons on mobile. Can we implement that here?

@dubielzyk Yes, that's possible. However, that doesn't solve the issue where user cannot exit the modal.

dubielzyk · 2020-04-17T08:29:46Z

However, that doesn't solve the issue where user cannot exit the modal.

Clicking confirm closes the screen so it should :)

yansern · 2020-04-17T08:31:47Z

Clicking confirm closes the screen so it should :)

Ah okay, I made some decisions based on some assumptions, e.g. onDomainConfirm event. I'm going to revert some changes made! Thanks!

yansern · 2020-04-17T09:04:20Z

onDomainConfirm removed. Temporary close button on modal removed.

yansern · 2020-04-17T09:22:15Z

Oops, I didn't foresee that gutenboarding content longer than the domain modal will coz the content behind the show. Need to rethink modal/scrollbar implementation.

It's actually not a bad idea if domain is a page by itself, just need to know which route to go back to?

Or quick and dirty is to css limit the height of page content when modal is rendered.

simison · 2020-04-17T09:43:37Z

This seems relevant; #40885 — i.e. ensuring that something is always selected before hitting "confirm". Can be separate PR.

simison · 2020-04-17T09:44:46Z

I didn't foresee that gutenboarding content longer than the domain modal will coz the content behind the show. Need to rethink modal/scrollbar implementation.

Another (perhaps simpler?) option could be signal the container to freeze scrollers underneath e.g. by applying wrapper is-modal-open or similar. Can be follow-up PR. :-)

sirreal

A first round of feedback to work on.

sirreal · 2020-04-17T09:10:36Z

client/landing/gutenboarding/components/domain-picker-modal/index.tsx

+/**
+ * External dependencies
+ */
+import React, { FunctionComponent } from 'react';


Let's start using this form everywhere:

Suggested change

import React, { FunctionComponent } from 'react';

import * as React from 'react';

Some context: https://twitter.com/sebmarkbage/status/1250284377138802689
This also seems to be the TypeScript convention.

client/landing/gutenboarding/components/domain-picker-modal/index.tsx

client/landing/gutenboarding/components/domain-picker-popover/index.tsx

config/development.json

client/landing/gutenboarding/components/domain-picker/index.tsx

sirreal · 2020-04-17T09:25:26Z

client/landing/gutenboarding/components/domain-picker-popover/index.tsx

+	return (
+		<div className="domain-picker-popover">
+			<Popover
+				focusOnMount={ isMobile ? 'container' : 'firstElement' }


I seem to get the input focused on mobile now. Is that a regression?

Fixed. useViewportMatch returns false on initial mount, causing the popover to focus on the search input, then after that, it updates itself to true, which at this point, the search input was already in focused. The solution was to keep the DomainPickerPopover component warm, mounted but renders nothing until isOpen flag is set to true. f81559a#diff-3acfaab9099bccb32e73498b4be055a4R41-R46

Oh, that's a shame. It looks like something that could be improved in useMediaQuery:

https://github.com/WordPress/gutenberg/blob/5494ff5e528fa7ebe2cb53bf33cf914de3405e60/packages/compose/src/hooks/use-media-query/index.js#L13

Created an issue WordPress/gutenberg#21676.

sirreal · 2020-04-17T09:27:34Z

client/landing/gutenboarding/components/domain-picker-button/index.tsx

@@ -2,18 +2,16 @@
 * External dependencies


It might be time to organize all these under a domain-picker directory:

domain-picker/ - index - re-exports whatever makes sense (modal, popover, the contents?) - modal - popover - …

Created an issue for this. #41221

client/landing/gutenboarding/components/domain-picker-modal/index.tsx

… has a scrollbar.

yansern · 2020-04-17T12:01:16Z

I didn't foresee that gutenboarding content longer than the domain modal will coz the content behind the show. Need to rethink modal/scrollbar implementation.

Another (perhaps simpler?) option could be signal the container to freeze scrollers underneath e.g. by applying wrapper is-modal-open or similar. Can be follow-up PR. :-)

Turns out to be an easy fix. react-modal lib is pretty good! c2154e7

yansern · 2020-04-17T12:08:37Z

Alright, these are fixed and we're ready for 2nd round of review:

all review notes from @sirreal
search input autofocusing when it shouldn't on mobile view.
more options showing in the middle of the popover when popover has a scrollbar.
long gutenboarding content visible in the background when modal is open.
react-modal showing appElement warning.

yansern · 2020-04-17T12:10:21Z

From this PR, we'll branch out 3 other PRs for:

domain connect
domain categories filter
mobile view for domain picker modal

alshakero

Love this! I left a few minor comments. And I found one issue regarding persistence that might be a bit complicated to fix, can be in another PR.

When you pick a domain and refresh the page, the domain is persisted alright, but the picker modal doesn't seem to be aware of this information and does a new search based on the site title. I'm not sure how can we go about fixing this.

client/landing/gutenboarding/components/domain-picker-modal/style.scss

alshakero · 2020-04-17T13:32:51Z

client/landing/gutenboarding/components/domain-picker-modal/style.scss

+.domain-picker-modal-overlay {
+	// Absolute positioning allows the modal
+	// to reuse the <body> element's scrollbar.
+	position: absolute;
+
+	// This positions the domain picker modal
+	// right below the gutenboarding header,
+	// keeping the header clickable.
+	top: 64px;
+	left: 0;
+
+	// Using min-height lets the overlay cover
+	// the entire viewport ensuring nothing behind
+	// it can be seen.
+	//
+	// When the domain picker's content is taller
+	// than the viewport height, it will expand taller
+	// than the provided min-height, triggering
+	// the appearance of the <body> element's scrollbar.
+	min-height: calc( 100vh - 64px );
+	width: 100%;
+
+	background: var( --studio-white );
+}


Beautiful comments. Thanks!

alshakero · 2020-04-17T13:33:30Z

client/landing/gutenboarding/components/domain-picker-modal/style.scss

+.domain-picker-modal {
+
+	.domain-picker__panel-row-main {
+		padding: 64px 88px;


These numbers seem a bit magical. Can comment on what they are and maybe use variables.

Thanks! They are intended to mimic onboarding block's margins and header margins. All ambigous numbers have been replaced with variables or proper mixins. e608b09

alshakero · 2020-04-17T13:45:55Z

client/landing/gutenboarding/components/domain-picker-popover/index.tsx

+	return (
+		<div className="domain-picker-popover">
+			<Popover
+				focusOnMount={ isMobile ? 'container' : 'firstElement' }


Created an issue WordPress/gutenberg#21676.

yansern · 2020-04-17T14:28:12Z

When you pick a domain and refresh the page, the domain is persisted alright, but the picker modal doesn't seem to be aware of this information and does a new search based on the site title. I'm not sure how can we go about fixing this.

Created a follow-up issue. #41228

yansern added 6 commits April 17, 2020 15:15

Added @types/react-modal.

bd09373

Added DomainPickerModal component.

23913e6

Refactor/split out DomainPickerPopover component.

e4d003e

Remove onClose event, just use onClick.

0693f13

Preparation work for domain categories & domain connect. Clean up sty…

f0a2c62

…lesheet.

Add a close button temporarily on the modal to be able to close the m…

cbd9cdc

…odal.

yansern requested review from a team as code owners April 17, 2020 07:41

yansern added [Goal] New Onboarding previously called Gutenboarding [Feature Group] Emails & Domains Features related to email integrations and domain management. labels Apr 17, 2020

yansern self-assigned this Apr 17, 2020

yansern added 2 commits April 17, 2020 16:57

Revert onDomainConfirm, confirm button closes domain picker.

0fa467f

Remove temporary close button from domain picker modal.

f22e6f7

yansern added 2 commits April 17, 2020 17:18

Bigger heading, same padding as other onboarding pages.

7aa5134

Hide domain picker button when modal is open.

1ddf02d

sirreal reviewed Apr 17, 2020

View reviewed changes

yansern mentioned this pull request Apr 17, 2020

Gutenboarding: Reorganize domain picker folder structure #41221

Closed

yansern added 3 commits April 17, 2020 18:54

Remove extra <div>.

4770de1

Spread props, import * from react.

74e2812

Fixed more options button showing in the middle of popover if popover…

e616558

… has a scrollbar.

yansern added 3 commits April 17, 2020 19:25

Use className.

3390dfd

Fixed regression where search input is focused on when mobile.

f81559a

Hide onboarding content when domain picker modal is open.

c2154e7

Fixed react modal appElement warning.

b2546f8

yansern mentioned this pull request Apr 17, 2020

Gutenboarding: Add TrainTracks events for domain picker #41173

Merged

3 tasks

Remove unused css.

9d4c05c

yansern added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 17, 2020

alshakero approved these changes Apr 17, 2020

View reviewed changes

Ensure no ambigous paddings & margins.

e608b09

yansern mentioned this pull request Apr 17, 2020

Gutenboarding: Domain picker modal search results not persistent across page refresh #41228

Closed

yansern merged commit 8cdc08e into master Apr 17, 2020

yansern deleted the add/gutenboarding-domain-picker-modal branch April 17, 2020 14:29

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 17, 2020

sgomes mentioned this pull request Apr 17, 2020

Fix Sass bug in gutenboarding domain picker modal #41232

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gutenboarding: Domain Picker Modal #41212

Gutenboarding: Domain Picker Modal #41212

yansern commented Apr 17, 2020 •

edited

Loading

matticbot commented Apr 17, 2020

matticbot commented Apr 17, 2020 •

edited

Loading

dubielzyk commented Apr 17, 2020

yansern commented Apr 17, 2020

dubielzyk commented Apr 17, 2020

yansern commented Apr 17, 2020

yansern commented Apr 17, 2020

yansern commented Apr 17, 2020

simison commented Apr 17, 2020 •

edited

Loading

simison commented Apr 17, 2020 •

edited

Loading

sirreal left a comment

sirreal Apr 17, 2020

sirreal Apr 17, 2020

yansern Apr 17, 2020 •

edited

Loading

sirreal Apr 17, 2020

alshakero Apr 17, 2020

sirreal Apr 17, 2020

yansern Apr 17, 2020

yansern commented Apr 17, 2020

yansern commented Apr 17, 2020

yansern commented Apr 17, 2020 •

edited

Loading

alshakero left a comment

alshakero Apr 17, 2020

alshakero Apr 17, 2020

yansern Apr 17, 2020

alshakero Apr 17, 2020

yansern commented Apr 17, 2020

	import React, { FunctionComponent } from 'react';
	import * as React from 'react';

Gutenboarding: Domain Picker Modal #41212

Gutenboarding: Domain Picker Modal #41212

Conversation

yansern commented Apr 17, 2020 • edited Loading

Changes proposed in this Pull Request

The component tree looks like this:

Here are the list of changes:

Other changes:

Testing instructions

Screenshot

matticbot commented Apr 17, 2020

matticbot commented Apr 17, 2020 • edited Loading

dubielzyk commented Apr 17, 2020

yansern commented Apr 17, 2020

dubielzyk commented Apr 17, 2020

yansern commented Apr 17, 2020

yansern commented Apr 17, 2020

yansern commented Apr 17, 2020

simison commented Apr 17, 2020 • edited Loading

simison commented Apr 17, 2020 • edited Loading

sirreal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yansern Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yansern commented Apr 17, 2020

yansern commented Apr 17, 2020

yansern commented Apr 17, 2020 • edited Loading

alshakero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yansern commented Apr 17, 2020

yansern commented Apr 17, 2020 •

edited

Loading

matticbot commented Apr 17, 2020 •

edited

Loading

simison commented Apr 17, 2020 •

edited

Loading

simison commented Apr 17, 2020 •

edited

Loading

yansern Apr 17, 2020 •

edited

Loading

yansern commented Apr 17, 2020 •

edited

Loading