Extract the search component into `@automattic/search` #46424

sarayourfriend · 2020-10-13T20:17:17Z

Rationale

We need to extract this component so that it can be used inside of the language-picker component. I've also moved it outside of @automattic/components for the same reasons laid out here: #46020 (comment)

Changes proposed in this Pull Request

Extract the search component from Calypso into a separate package
Removes analytics from the component. Consumers will need to implement their own using the appropriate hooks.

This is more or less a 1-to-1 reimplementation of the existing component. There are some differences, however:

Updated ref usage
Using WordPress components rather than Calypso components and icons
Using TypeScript and fixing some potential coding errors
Add a wrapping noop form to cause iOS to display a "Search" button rather than "return"

Testing instructions

Run the new storybook command yarn run search:storybook:start and ensure the examples work as expected and cover the different use cases

Note: I didn't port over the existing tests because they aren't really that useful of tests.

Questions

The way this component is written it can go from controlled to uncontrolled (if you start to pass in value later in the component lifecycle. We could vastly simplify this component if we turned it into a fully uncontrolled component. For example, if we did this, we could remove the derived state logic and the "responsiveness" of the component to certain props changing (like do we really need to be able to handle changes in delaySearch? That seems like a waste and a supremely unlikely edge case usage). Shall we move forward with turning this component into an uncontrolled component (i.e., by not responding to changes in "static" props and not allowing value to be passed in?)

Fixes #

matticbot · 2020-10-13T20:17:20Z

Test live: https://calypso.live/?branch=try/extract-search

a8ci18n · 2020-10-15T16:59:40Z

packages/search/src/search.tsx

+					tabIndex={ enableOpenIcon ? 0 : undefined }
+					onKeyDown={ enableOpenIcon ? this.openListener : undefined }
+					aria-controls={ 'search-component-' + this.instanceId }
+					aria-label={ __( 'Open Search' ) }


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 31 times:
translate( 'Open Search', { context: 'button label'} ) ES Score: 14

matticbot · 2020-10-15T17:08:22Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

tyxla

Nice work @saramarcondes.

I'm definitely interested to see a simplification of this component, but it's probably best to be done in another PR. Maybe we could write some tests and then do the refactor.

Yet, I wonder if refactoring it as uncontrolled makes sense - are we going to keep all the functionality that it already offers?

I've left some thoughts, but this generally looks like pretty solid work!

packages/search/src/search.stories.tsx

packages/search/src/search.tsx

tyxla · 2020-10-16T11:06:32Z

packages/search/src/search.tsx

+	};
+
+	UNSAFE_componentWillReceiveProps( nextProps: Props ) {
+		if ( this.props.isOpen !== nextProps.isOpen ) {


This feels a bit flawed. The component state can be changed by user intent but could be overridden by another prop change, feels a bit off. I'd expect isOpen to be used only for the initial state. Is there a real use case where this behavior is necessary?

Maybe there is a valid use case where the parent wants to close the search, no matter what the internal state is?

These cases would maybe be covered if the search component auto-closed when it loses focus -- means that another UI element is now active.

These cases would maybe be covered if the search component auto-closed when it loses focus -- means that another UI element is now active.

For this to work we'd need to always auto-focus. I'm not opposed to that given that a similar component in the new P2 follows this behavior but my understanding is that auto-focusing is undesirable for accessibility reasons. Maybe that doesn't apply for when you're "auto-focusing" on a new element after activating a button that makes the new element appear?

As far as I understand, auto-focusing is always intrusive from an accessibility point of view. I might be wrong, of course - @diegohaz would know best.

Auto-focus is okay if followed by a user action that makes sense. For example, clicking on a <label> should focus on the input; opening a dialog should move focus into it; deleting the currently focused row in a table could move focus somewhere else (like the next or previous rows).

In this component, I would expect that the input would be focused after clicking on the search icon.

jsnajdr · 2020-10-16T11:55:30Z

packages/search/src/search.tsx

+			isOpen: this.props.isOpen || false,
+		} );
+
+		if ( this.searchInput?.current ) {


this.searchInput always exists and always has the current property. It's the value of current that can be null. The optional chaining is not needed.

jsnajdr · 2020-10-16T11:55:54Z

packages/search/src/search.tsx

+			this.searchInput?.current?.blur();
+			this.openIcon?.current?.focus();
+		} else {
+			this.searchInput?.current?.focus();


Same here: only the ?. after current needs to be optional.

jsnajdr · 2020-10-16T11:58:59Z

packages/search/src/search.tsx

+	};
+
+	UNSAFE_componentWillReceiveProps( nextProps: Props ) {
+		if ( this.props.isOpen !== nextProps.isOpen ) {


Maybe there is a valid use case where the parent wants to close the search, no matter what the internal state is?

These cases would maybe be covered if the search component auto-closed when it loses focus -- means that another UI element is now active.

packages/search/src/search.tsx

jsnajdr · 2020-10-16T12:04:40Z

packages/search/src/search.tsx

+		hasFocus: this.props.autoFocus,
+	};
+
+	UNSAFE_componentWillReceiveProps( nextProps: Props ) {


A new component shouldn't use the legacy method. The new getDerivedStateFromProps method is preferred. Or even better, do a functional component with hooks.

This component cannot actually use getDerivedStateFromProps because it's being controlled externally and internally. It specifically needs to respond to changes in isOpen. If we turn this into an uncontrolled component then we can remove this logic and replace it with a defaultIsOpen as you suggested in another comment. Then consumers that need the component to respond to external isOpen may add a key prop that depends on their isOpen value to force the component to re-mount with the new default value.

diegohaz · 2020-10-16T16:50:06Z

Maybe it's out of the scope of this PR since this is just a code extraction. But I wonder why the buttons are <div>s and not <button>s?

sarayourfriend · 2020-10-16T17:11:45Z

But I wonder why the buttons are
s and not s?

I think its within scope for this PR. I'd like to extract and make significant improvements wherever possible! I'll update that, thanks for pointing it out. (Plus we can use WordPress Button... yet another win 😀)

wp-desktop

WordPress Desktop CI Failure for job "wp-desktop-mac".

@saramarcondes please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

wp-desktop ci passing, closing review

a8ci18n · 2020-10-19T18:11:17Z

packages/search/src/search.tsx

+				tabIndex={ enableOpenIcon ? 0 : undefined }
+				onKeyDown={ enableOpenIcon ? this.openListener : undefined }
+				aria-controls={ 'search-component-' + this.instanceId }
+				aria-label={ __( 'Open Search' ) }


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 31 times:
translate( 'Open Search', { context: 'button label'} ) ES Score: 14

wp-desktop

WordPress Desktop CI Failure for job "wp-desktop-mac".

@saramarcondes please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

Also add a noop wrapping form to cause soft-keyboards in iOS to display a "search" button instead of "return": https://stackoverflow.com/a/26287843

wp-desktop ci passing, closing review

wp-desktop

WordPress Desktop CI Failure for job "wp-desktop-mac".

@saramarcondes please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

wp-desktop ci passing, closing review

tyxla

Looking really well and testing great!

I've left some nits, but nothing blocking IMHO.

I think it's already in a great state and we can address any potential improvement in subsequent PRs.

Great work @saramarcondes!

tyxla · 2020-10-20T11:00:31Z

packages/search/package.json

+	},
+	"devDependencies": {
+		"@storybook/addon-actions": "^5.3.18",
+		"enzyme": "^3.11.0"


Since we don't have any tests yet, it seems we can remove enzyme from the list of dependencies, no?

tyxla · 2020-10-20T11:04:33Z

packages/search/package.json

@@ -0,0 +1,52 @@
+{
+	"name": "@automattic/search",
+	"version": "1.0.0-alpha.3",


Nit: we haven't released that yet, should it be 1.0.0-alpha or beta or even just 1.0.0?

tyxla · 2020-10-20T11:14:48Z

packages/search/src/style.scss

+	}
+}
+
+.search__input-fade .search__text-overlay {


I wonder if it makes sense to have all the styles nested in .search, to ensure we're not bleeding any styles beyond the component.

Also, while on that topic, I wonder if it makes sense to change the wrapper .search to a more specific classname. .search seems a bit too common, and IMHO there's a big chance to run into conflicts.

packages/search/CHANGELOG.md

tyxla · 2020-10-20T11:21:42Z

packages/search/CHANGELOG.md

@@ -0,0 +1,3 @@
+## 1.0.0
+
+- Add Search component


Should we mention that this based on the existing code in Calypso?

tyxla · 2020-10-20T11:22:53Z

packages/search/src/search.tsx

+/**
+ * External dependencies
+ */
+import React, { KeyboardEvent, MouseEvent, FocusEvent, FormEvent, ChangeEvent } from 'react';


Opinionated nit: would be really great if we can sort the imports, both when they're multiple imports from the same module and multiple imports on multiple lines. Makes it more readable IMHO.

Shall we enable an ESLint rule for sorted imports?

My personal opinion is YES, but maybe now it's not the time. With the new rule that we recently enabled to make all imports relative to the root of Calypso, any change to existing files will be a mess for contributors.

I say let's reconsider doing at some point in the future.

/cc @scinos

Plenty of options if we decide go that route https://www.npmjs.com/search?q=eslint%20sort%20import

wp-desktop

WordPress Desktop CI Failure for job "wp-desktop-mac".

@saramarcondes please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

wp-desktop ci passing, closing review

jsnajdr · 2020-10-30T08:58:51Z

packages/search/package.json

+	"scripts": {
+		"clean": "npx rimraf dist && tsc --build --clean",
+		"prepublish": "yarn run clean",
+		"prepare": "transpile && tsc && copy-assets"


Hi @saramarcondes 👋 I noticed that even though the search package is written fully in TypeScript, the transpilation for the dist directories is done by Babel. The TypeScript compiler is used only to emit the type declarations.

Was this an intentional decision or is it some copy-paste accident? The compilation could be done 100% by the TypeScript compiler, with a pair of tsconfig.json and tsconfig-cjs.json config files. The @automattic/data-stores package does that, for example.

This is an accident of copy and paste from the way the components package is configured. Please feel free to open a PR to fix this if you can, I’m not really familiar with how it would work without Babel...

Side note: the language picker package probably has the same problem.

Thanks, I'll patch both packages in a PR 👍

sarayourfriend force-pushed the try/extract-search branch 5 times, most recently from eab26ea to 28fe17d Compare October 15, 2020 00:19

sarayourfriend changed the title ~~Try extracting search component~~ Extract the search component into @automattic/search Oct 15, 2020

sarayourfriend marked this pull request as ready for review October 15, 2020 16:44

sarayourfriend requested a review from tyxla October 15, 2020 16:44

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

a8ci18n reviewed Oct 15, 2020

View reviewed changes

sarayourfriend requested a review from a team October 15, 2020 17:15

sarayourfriend mentioned this pull request Oct 15, 2020

Audit experimental features in WordPress Components Gallery #46496

Closed

10 tasks

tyxla reviewed Oct 16, 2020

View reviewed changes

jsnajdr reviewed Oct 16, 2020

View reviewed changes

wp-desktop previously requested changes Oct 19, 2020

View reviewed changes

sarayourfriend force-pushed the try/extract-search branch from 39e04ee to b99034d Compare October 19, 2020 18:01

a8ci18n reviewed Oct 19, 2020

View reviewed changes

wp-desktop previously requested changes Oct 19, 2020

View reviewed changes

sarayourfriend added 7 commits October 19, 2020 11:49

Try extracting search component

226a2b6

Clean types and remove complexities around debouncing

0a752e0

Fix searching

89d759d

Clean up comments and deps

966e7dc

Replace mistakingly removed styles

238f5a1

Add i18n documentation

a0c8d11

Suppress type errors

ab016ed

sarayourfriend added 8 commits October 19, 2020 11:49

Use TextControl

3d8fb56

Also add a noop wrapping form to cause soft-keyboards in iOS to display a "search" button instead of "return": https://stackoverflow.com/a/26287843

Use actual button instead of div and fix compact icon placement

5031caf

Remove autoFocus from example

f9493df

Fix type errors

9fa8e9b

Use a regular input to allow for stable refs

6e349fd

Convert to uncontrolled component

6631056

Remove unnecessary null checks

08c3bd4

Clean up and add overlay styling example

fe517fd

sarayourfriend force-pushed the try/extract-search branch from b99034d to fe517fd Compare October 19, 2020 18:49

sarayourfriend requested review from tyxla and jsnajdr October 19, 2020 20:12

Fix rtl value

3271f9d

wp-desktop previously requested changes Oct 19, 2020

View reviewed changes

Remove unnecessary extra element

bc078b9

tyxla approved these changes Oct 20, 2020

View reviewed changes

tyxla mentioned this pull request Oct 20, 2020

Extract Gutenboarding language picker into @automattic/language-picker #46020

Merged

sarayourfriend added 5 commits October 20, 2020 07:47

Remove unnecessary deps and fix version

587db54

Use more specific classnames

ba4a537

Sort imports

790b225

Nest all styles

54a1b3b

Update CHANGELOG

65cf3b9

wp-desktop previously requested changes Oct 20, 2020

View reviewed changes

sarayourfriend merged commit 3bee2df into master Oct 21, 2020

sarayourfriend deleted the try/extract-search branch October 21, 2020 19:06

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

jsnajdr reviewed Oct 30, 2020

View reviewed changes

jsnajdr mentioned this pull request Nov 2, 2020

Optimize how TypeScript-authored packages are built #47002

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract the search component into `@automattic/search` #46424

Extract the search component into `@automattic/search` #46424

sarayourfriend commented Oct 13, 2020 •

edited

Loading

matticbot commented Oct 13, 2020

a8ci18n Oct 15, 2020

matticbot commented Oct 15, 2020

tyxla left a comment •

edited

Loading

tyxla Oct 16, 2020

jsnajdr Oct 16, 2020

sarayourfriend Oct 16, 2020

tyxla Oct 16, 2020

diegohaz Oct 16, 2020

jsnajdr Oct 16, 2020

jsnajdr Oct 16, 2020

jsnajdr Oct 16, 2020

jsnajdr Oct 16, 2020

sarayourfriend Oct 19, 2020

diegohaz commented Oct 16, 2020

sarayourfriend commented Oct 16, 2020

wp-desktop left a comment

a8ci18n Oct 19, 2020

wp-desktop left a comment

wp-desktop left a comment

tyxla left a comment

tyxla Oct 20, 2020

tyxla Oct 20, 2020

tyxla Oct 20, 2020

tyxla Oct 20, 2020

tyxla Oct 20, 2020

sarayourfriend Oct 20, 2020 •

edited

Loading

tyxla Oct 21, 2020

sarayourfriend Oct 21, 2020

scinos Oct 21, 2020

wp-desktop left a comment

jsnajdr Oct 30, 2020 •

edited

Loading

sarayourfriend Oct 30, 2020

jsnajdr Oct 30, 2020

Extract the search component into @automattic/search #46424

Extract the search component into @automattic/search #46424

Conversation

sarayourfriend commented Oct 13, 2020 • edited Loading

Rationale

Changes proposed in this Pull Request

Testing instructions

Questions

matticbot commented Oct 13, 2020

Choose a reason for hiding this comment

matticbot commented Oct 15, 2020

tyxla left a comment • 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

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

Choose a reason for hiding this comment

diegohaz commented Oct 16, 2020

sarayourfriend commented Oct 16, 2020

wp-desktop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wp-desktop left a comment

Choose a reason for hiding this comment

wp-desktop left a comment

Choose a reason for hiding this comment

tyxla 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

Choose a reason for hiding this comment

sarayourfriend Oct 20, 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

wp-desktop left a comment

Choose a reason for hiding this comment

jsnajdr Oct 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Extract the search component into `@automattic/search` #46424

Extract the search component into `@automattic/search` #46424

sarayourfriend commented Oct 13, 2020 •

edited

Loading

tyxla left a comment •

edited

Loading

sarayourfriend Oct 20, 2020 •

edited

Loading

jsnajdr Oct 30, 2020 •

edited

Loading