Search: i18nify the component #46825

sarayourfriend · 2020-10-27T19:55:27Z

Changes proposed in this Pull Request

Use withI18n rather than using wp/i18n directly. Aligns with how we're doing things in the LanguagePicker component.

Testing instructions

Run the storybook and ensure the component still renders and works correctly
Use the language picker in Calypso at /me/account and ensure search still behaves as expected on desktop and mobile

Fixes #

matticbot · 2020-10-27T19:55:28Z

Test live: https://calypso.live/?branch=add/i18nify-search-component

matticbot · 2020-10-28T17:58:53Z

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

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

name                 parsed_size           gzip_size
entry-gutenboarding       +507 B  (+0.0%)     +167 B  (+0.0%)
entry-main                +282 B  (+0.0%)     +101 B  (+0.0%)

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

Sections (~168 bytes added 📈 [gzipped])

name         parsed_size           gzip_size
woocommerce       +225 B  (+0.0%)      +50 B  (+0.0%)
settings          +225 B  (+0.0%)      +52 B  (+0.0%)
account           +225 B  (+0.0%)      +66 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 (~244 bytes added 📈 [gzipped])

name                                parsed_size           gzip_size
async-load-quick-language-switcher       +225 B  (+0.2%)      +66 B  (+0.2%)
async-load-design-playground             +225 B  (+0.0%)      +57 B  (+0.0%)
async-load-design-blocks                 +225 B  (+0.0%)      +60 B  (+0.0%)
async-load-design                        +225 B  (+0.0%)      +61 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.

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.

tyxla

Hey @saramarcondes is this ready for review or are you still working on it?

tyxla · 2020-10-29T12:19:27Z

packages/search/src/search.tsx

@@ -310,7 +311,7 @@ class Search extends React.Component< Props, State > {

 	render() {
 		const searchValue = this.state.keyword;
-		const placeholder = this.props.placeholder || __( 'Search…' );
+		const placeholder = this.props.placeholder || this.props.__( 'Search…', __i18n_text_domain__ );


I was wondering why we need the textdomain argument now.

I think we need it in case the search component is used outside of the default context. And in this case we're receiving __ from withI18n so we need to still supply a context, right?

wp-desktop ci passing, closing review

tyxla

I seem to be getting an error when trying to start search's storybook:

Any idea what might be going on?

sarayourfriend · 2020-11-02T16:50:03Z

@tyxla I'm having the same issue. I have no idea why that is happening 🤔 Maybe @scinos could help?

tyxla · 2020-11-03T10:41:49Z

Maybe we need to add ts-loader and react-docgen-typescript-loader as dependencies, too?

sarayourfriend · 2020-11-03T14:42:37Z

I'll try that but storybook worked before these changes, why should we need new dependencies based on these changes? 🤔

By the way, where did react-docgen-typescript-loader come from? I'm not seeing that in the error at all.

tyxla · 2020-11-04T08:10:57Z

I'll try that but storybook worked before these changes, why should we need new dependencies based on these changes? 🤔

Not sure, perhaps another dependency or peer dependency that was updated needs those. Or we changed something with our TS config along the way. But the fact that storybook doesn't work for components package either means that it's not related to any of the component work done here.

By the way, where did react-docgen-typescript-loader come from? I'm not seeing that in the error at all.

I saw an error message that it's missing when I added ts-loader as dependency. I didn't go any further, but wanted to point that out.

tyxla · 2020-11-04T14:42:02Z

I've tinkered with fixing storybook in #47098

tyxla · 2020-11-06T14:31:38Z

Storybook builds, but I'm getting an error when opening the example in the browser 😬 :

tyxla · 2020-11-06T14:32:04Z

Is it possible this is something coming from the withI18n HoC?

sarayourfriend · 2020-11-06T15:41:22Z

@tyxla yes. This is the same issue we ran into with the LanguagePicker which led me to delete the storybook sigh. is-shallow-equal is for whatever reason not compatible with Storybook's build process.

tyxla · 2020-11-09T11:17:56Z

@tyxla yes. This is the same issue we ran into with the LanguagePicker which led me to delete the storybook sigh. is-shallow-equal is for whatever reason not compatible with Storybook's build process.

Is this because @wordpress/is-shallow-equal uses CJS module.export? Can this be fixed if we change those to ESM there? Do you know if there's a specific reason why we're using CJS in @wordpress/is-shallow-equal? cc @sirreal

sirreal · 2020-11-09T11:37:12Z

is-shallow-equal doesn't require build or compilation. Its source is designed to be used via node or bundled for the browser without modification. There's some conversation about that here: WordPress/gutenberg#18942 (comment)

I'm sure Gutenberg would be open to revisiting that decision especially as ES Modules mature (specifically in node).

tyxla · 2020-11-09T13:49:46Z

is-shallow-equal doesn't require build or compilation. Its source is designed to be used via node or bundled for the browser without modification. There's some conversation about that here: WordPress/gutenberg#18942 (comment)

That makes sense, thanks for the context @sirreal 🙌

I'm sure Gutenberg would be open to revisiting that decision especially as ES Modules mature (specifically in node).

That would be an interesting avenue to explore, @saramarcondes.

Alternatively, we could update react-i18n to manually create the withI18n HoC instead of using createHigherOrderComponent() from @wordpress/compose, which seems to be what needs is-shallow-equal right now.

sarayourfriend · 2020-11-09T17:17:45Z

@tyxla and @sirreal Thanks for the suggestions and information. I've opened a PR here (WordPress/gutenberg#26833) to convert is-shallow-equal to ESM.

sarayourfriend · 2020-12-18T20:42:30Z

@tyxla Any chance you could re-review this? Thanks!

tyxla

Storybook builds now, but I'm getting this error when trying to run it in the browser:

Furthermore, the @automattic/search package is now used in the language picker. Perhaps this PR needs to be rebased, and test instructions expanded to account for that usage?

tyxla

Looks good and works well 👍 (noting that I rebased it locally before testing)

sarayourfriend requested a review from tyxla October 27, 2020 19:55

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 27, 2020

sarayourfriend self-assigned this Oct 27, 2020

sarayourfriend added the Components label Oct 27, 2020

wp-desktop previously requested changes Oct 28, 2020

View reviewed changes

tyxla reviewed Oct 29, 2020

View reviewed changes

sarayourfriend marked this pull request as ready for review October 29, 2020 18:19

tyxla reviewed Nov 2, 2020

View reviewed changes

tyxla mentioned this pull request Nov 4, 2020

Packages: Fix components and search storybook #47098

Merged

sarayourfriend force-pushed the add/i18nify-search-component branch from e82f4f6 to 9168279 Compare November 4, 2020 16:08

sarayourfriend requested a review from tyxla November 5, 2020 19:15

sarayourfriend changed the base branch from master to trunk November 20, 2020 16:11

sarayourfriend force-pushed the add/i18nify-search-component branch from 9168279 to b3fdb03 Compare December 18, 2020 19:38

tyxla reviewed Dec 22, 2020

View reviewed changes

tyxla approved these changes Dec 23, 2020

View reviewed changes

sarayourfriend added 5 commits December 24, 2020 07:24

Search: i18nify the component

799a746

Fix storybook import

4085c2e

Don't use TS for storybook examples

ce0ccc8

Fix various type issues causing improper export

1ae4523

Fix storybook

e84df21

sarayourfriend force-pushed the add/i18nify-search-component branch from 59576cc to e84df21 Compare December 24, 2020 15:24

sarayourfriend merged commit 8909f2e into trunk Dec 24, 2020

sarayourfriend deleted the add/i18nify-search-component branch December 24, 2020 16:01

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

tjcafferkey mentioned this pull request Jan 6, 2021

Editing Toolkit: Update to 2.8.17 #48693

Merged

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search: i18nify the component #46825

Search: i18nify the component #46825

sarayourfriend commented Oct 27, 2020 •

edited

Loading

matticbot commented Oct 27, 2020

matticbot commented Oct 28, 2020 •

edited

Loading

wp-desktop left a comment

tyxla left a comment

tyxla Oct 29, 2020

sarayourfriend Oct 29, 2020

tyxla left a comment

sarayourfriend commented Nov 2, 2020

tyxla commented Nov 3, 2020

sarayourfriend commented Nov 3, 2020 •

edited

Loading

tyxla commented Nov 4, 2020

tyxla commented Nov 4, 2020

tyxla commented Nov 6, 2020

tyxla commented Nov 6, 2020

sarayourfriend commented Nov 6, 2020

tyxla commented Nov 9, 2020

sirreal commented Nov 9, 2020

tyxla commented Nov 9, 2020

sarayourfriend commented Nov 9, 2020

sarayourfriend commented Dec 18, 2020

tyxla left a comment

tyxla left a comment •

edited

Loading

Search: i18nify the component #46825

Search: i18nify the component #46825

Conversation

sarayourfriend commented Oct 27, 2020 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Oct 27, 2020

matticbot commented Oct 28, 2020 • edited Loading

wp-desktop left a comment

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

tyxla Oct 29, 2020

Choose a reason for hiding this comment

sarayourfriend Oct 29, 2020

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

sarayourfriend commented Nov 2, 2020

tyxla commented Nov 3, 2020

sarayourfriend commented Nov 3, 2020 • edited Loading

tyxla commented Nov 4, 2020

tyxla commented Nov 4, 2020

tyxla commented Nov 6, 2020

tyxla commented Nov 6, 2020

sarayourfriend commented Nov 6, 2020

tyxla commented Nov 9, 2020

sirreal commented Nov 9, 2020

tyxla commented Nov 9, 2020

sarayourfriend commented Nov 9, 2020

sarayourfriend commented Dec 18, 2020

tyxla left a comment

Choose a reason for hiding this comment

tyxla left a comment • edited Loading

Choose a reason for hiding this comment

sarayourfriend commented Oct 27, 2020 •

edited

Loading

matticbot commented Oct 28, 2020 •

edited

Loading

sarayourfriend commented Nov 3, 2020 •

edited

Loading

tyxla left a comment •

edited

Loading