Signup: Use new search for domains signup by sarayourfriend · Pull Request #51718 · Automattic/wp-calypso

sarayourfriend · 2021-04-06T18:37:18Z

Changes proposed in this Pull Request

Use new search component for domains during signup

Testing instructions

Go to /start/domains and ensure that search continues to work as expected.

Note: there's a usage of the ref that is assigned to the search to try to focus the element... however, I could never get this to work. It seems like the example suggestions is fully disabled so I'm going to open another PR to remove it.

matticbot · 2021-04-06T18:37:20Z

Test live: https://calypso.live/?branch=add/use-search-component-for-search-card

matticbot · 2021-04-06T18:46:14Z

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

App Entrypoints (~9 bytes removed 📉 [gzipped])

Details

name                 parsed_size           gzip_size
entry-main                -174 B  (-0.0%)      -14 B  (-0.0%)
entry-gutenboarding        +17 B  (+0.0%)      +22 B  (+0.0%)

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

Sections (~12076 bytes added 📈 [gzipped])

Details

name            parsed_size           gzip_size
domains            +65505 B  (+5.5%)   +16161 B  (+5.4%)
signup              +2136 B  (+0.9%)     +201 B  (+0.3%)
devdocs              +324 B  (+0.2%)      +33 B  (+0.1%)
people               +183 B  (+0.1%)     +665 B  (+0.7%)
email                -183 B  (-0.0%)    -1077 B  (-0.9%)
plans                -128 B  (-0.0%)     -866 B  (-0.4%)
purchases            -103 B  (-0.0%)     +500 B  (+0.1%)
site-purchases        +86 B  (+0.0%)     -168 B  (-0.1%)
settings              +17 B  (+0.0%)      +10 B  (+0.0%)
plugins               +17 B  (+0.0%)       +4 B  (+0.0%)
account               +17 B  (+0.0%)      +11 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~5370 bytes added 📈 [gzipped])

Details

name                                parsed_size            gzip_size
async-load-signup-steps-domains       +111575 B  (+42.2%)   +35488 B  (+52.3%)
async-load-design-playground             +200 B   (+0.0%)     +875 B   (+0.2%)
async-load-design-blocks                 +200 B   (+0.0%)     +826 B   (+0.1%)
async-load-design                        +200 B   (+0.0%)     +875 B   (+0.2%)
async-load-quick-language-switcher        +17 B   (+0.0%)       +5 B   (+0.0%)
async-load-automattic-search              +17 B   (+0.0%)       +6 B   (+0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

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.

noahtallen

This tests well for me, though the e2e failures seem like they could be related. I also noticed this JS error, not sure if it's related:

tyxla

I've noticed this introduces some color inconsistencies to the focused state:

Before:

After:

Also, could there be some props that need to be updated/removed as well? initialValue looks like one of them.

tyxla · 2021-04-07T05:22:08Z

client/components/domains/register-domain-step/index.jsx

 import { domainAvailability } from 'calypso/lib/domains/constants';
 import { getAvailabilityNotice } from 'calypso/lib/domains/registration/availability-messages';
-import Search from 'calypso/components/search';
+import Search from '@automattic/search';


Let's move this to external dependencies.

tyxla · 2021-04-07T05:28:41Z

client/components/domains/register-domain-step/index.jsx

 					<CompactCard className="register-domain-step__search-card">
 						<Search
-							additionalClasses={ this.state.clickedExampleSuggestion ? 'is-refocused' : undefined }
+							className={ this.state.clickedExampleSuggestion ? 'is-refocused' : undefined }


There seems to be a initialValue prop that the search package doesn't support.

Also, we're passing a ref, but we also create new refs in our component. Could that cause a regression?

There seems to be a initialValue prop that the search package doesn't support.

Ah yes I think this was renamed to defaultValue, good catch, thanks.

Also, we're passing a ref, but we also create new refs in our component. Could that cause a regression?

What do you mean by this? The ref that is passed get's handled by useImperativeHandle and duplicates the methods that were being used on the old component by the contexts where refs were being passed (mostly focus to be honest).

Scratch that, I don't think it's a concern anyway.

sarayourfriend · 2021-04-07T14:22:52Z

@noahtallen e2e failures are definitely related 🙂

sarayourfriend · 2021-04-07T19:04:03Z

E2e failures fixed as well as the focus styles. Everything should be tip top shape now 😁

noahtallen

lgtm! There's a flaky e2e test related to gutenboarding domains, but it seems to be passing for a lot of the commits here... so probably unrelated

tyxla

👍

sarayourfriend · 2021-04-08T14:27:31Z

I'm requesting from @Automattic/cobalt just for good measure 😁

klimeryk

Thanks for the ping! (and sorry for the late reply, we've had a lot of organizational changes recently).

I've tested as many flows as I could:

/domains
signup
launch flow

And looks good!

I've only noticed a very small issue, probably with the component itself. The X button does not have a focus style in the new component.

Compare the old one:

Screen.Recording.2021-04-14.at.17.21.47.mov

With the new one:

Screen.Recording.2021-04-14.at.17.22.05.mov

Definitely not a blocker.

One flow that seemed to have a slight regression is the reskinned signup (see the reskinSignupFlow A/B test), but that has been paused (#46477), so probably not a blocker (cc @aneeshd16)

Production:

PR:

It's a minor visual regression, so either way should be fine to merge as-is.

sarayourfriend added 2 commits April 6, 2021 11:35

Render children into the search area

9cf423d

Signup: Use new search for domains signup

ff24a40

sarayourfriend requested a review from a team April 6, 2021 18:37

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

sarayourfriend self-assigned this Apr 6, 2021

sarayourfriend added the Janitorial label Apr 6, 2021

noahtallen reviewed Apr 6, 2021

View reviewed changes

tyxla reviewed Apr 7, 2021

View reviewed changes

sarayourfriend added 2 commits April 7, 2021 10:54

Update e2e test selectors for the new component

1cb5412

Fix focused and other styles and style-lint errors

0b08078

sarayourfriend added 2 commits April 7, 2021 12:06

Use correct prop for default value

bfa15ef

Move dependency to external

f3b13e7

noahtallen approved these changes Apr 7, 2021

View reviewed changes

tyxla approved these changes Apr 8, 2021

View reviewed changes

sarayourfriend requested a review from a team April 8, 2021 14:27

klimeryk requested review from a team and dzver and removed request for a team and dzver April 10, 2021 15:25

klimeryk approved these changes Apr 14, 2021

View reviewed changes

Fix focus styles for button

87226c1

sarayourfriend merged commit 27c97f5 into trunk Apr 15, 2021

sarayourfriend deleted the add/use-search-component-for-search-card branch April 15, 2021 17:52

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 15, 2021

klimeryk mentioned this pull request Apr 21, 2021

Shift + letter in middle of domain search string sends cursor to end of string #48225

Closed

sirbrillig mentioned this pull request Sep 14, 2021

Add automattic/search to client dependencies #56245

Merged

Conversation

sarayourfriend commented Apr 6, 2021

Changes proposed in this Pull Request

Testing instructions

Uh oh!

matticbot commented Apr 6, 2021

Uh oh!

matticbot commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noahtallen left a comment

Choose a reason for hiding this comment

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

tyxla Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

tyxla Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

sarayourfriend Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyxla Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

sarayourfriend commented Apr 7, 2021

Uh oh!

sarayourfriend commented Apr 7, 2021

Uh oh!

noahtallen left a comment

Choose a reason for hiding this comment

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

sarayourfriend commented Apr 8, 2021

Uh oh!

klimeryk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

matticbot commented Apr 6, 2021 •

edited

Loading

sarayourfriend Apr 7, 2021 •

edited

Loading