Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add editor in reader behind a feature flag #76035

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

creativecoder
Copy link
Contributor

@creativecoder creativecoder commented Apr 20, 2023

Proposed Changes

Adds an instance of isolated-block-editor behind a feature flag, reader/editor.

This is meant to be a starting point that we can build on. See pe7F0s-Kk-p2

Testing Instructions

  • Open /read on calypso.localhost
  • Click on the editor and create a post.
  • Click publish to publish the post on your primary site.

@creativecoder creativecoder self-assigned this Apr 20, 2023
@creativecoder creativecoder requested a review from a team as a code owner April 20, 2023 14:08
@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 20, 2023
@creativecoder creativecoder force-pushed the add/editor-in-reader-behind-flag branch from 1d181cc to 83a02cd Compare April 20, 2023 14:09
@matticbot
Copy link
Contributor

matticbot commented Apr 20, 2023

@matticbot
Copy link
Contributor

matticbot commented Apr 20, 2023

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

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

name                   parsed_size           gzip_size
entry-login               -14241 B  (-1.0%)    -4054 B  (-1.0%)
entry-subscriptions       -14071 B  (-0.9%)    -3822 B  (-0.9%)
entry-main                -13866 B  (-0.8%)    -4006 B  (-0.8%)
entry-stepper             -13503 B  (-0.6%)    -3758 B  (-0.6%)
entry-domains-landing       +444 B  (+0.1%)      +33 B  (+0.0%)
entry-browsehappy           +444 B  (+0.4%)      +33 B  (+0.1%)

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

Sections (~22778 bytes added 📈 [gzipped])

name                             parsed_size           gzip_size
pattern-assembler-step              +52763 B  (+3.7%)   +12312 B  (+3.0%)
import-flow                         +52088 B  (+1.8%)   +12183 B  (+1.5%)
reader                               +1970 B  (+0.3%)     +421 B  (+0.2%)
theme                                 +216 B  (+0.0%)      +47 B  (+0.0%)
people                                -171 B  (-0.0%)      -43 B  (-0.0%)
themes                                +129 B  (+0.0%)      +42 B  (+0.0%)
with-theme-assembler-flow             +108 B  (+1.7%)      +13 B  (+0.6%)
site-setup-flow                       +108 B  (+0.0%)     +150 B  (+0.0%)
site-logs                             -103 B  (-0.0%)       -8 B  (-0.0%)
settings-writing                      -103 B  (-0.0%)      -25 B  (-0.0%)
settings-security                     -103 B  (-0.0%)      -13 B  (-0.0%)
settings-reading                      -103 B  (-0.0%)      -13 B  (-0.0%)
settings-performance                  -103 B  (-0.0%)      -13 B  (-0.0%)
settings-discussion                   -103 B  (-0.0%)      -13 B  (-0.0%)
settings                              -103 B  (-0.0%)       +8 B  (+0.0%)
marketing                             -103 B  (-0.0%)      -13 B  (-0.0%)
earn                                  -103 B  (-0.0%)      -13 B  (-0.0%)
newsletter-flow                        -95 B  (-1.1%)      -33 B  (-1.2%)
update-design-flow                     +72 B  (+0.0%)       -1 B  (-0.0%)
signup                                 +72 B  (+0.0%)       -1 B  (-0.0%)
jetpack-connect                        +72 B  (+0.0%)       -1 B  (-0.0%)
home                                   +72 B  (+0.0%)       -1 B  (-0.0%)
accept-invite                          +72 B  (+0.0%)       -1 B  (-0.0%)
jetpack-cloud-plugin-management        +67 B  (+0.0%)     +699 B  (+0.2%)
subscribers                            +65 B  (+0.0%)      -59 B  (-0.1%)
write-flow                             +60 B  (+0.0%)       -4 B  (-0.0%)
videopress-flow                        +60 B  (+0.0%)       -4 B  (-0.0%)
link-in-bio-tld-flow                   +60 B  (+0.0%)       -4 B  (-0.0%)
free-flow                              +60 B  (+0.0%)       -4 B  (-0.0%)
build-flow                             +60 B  (+0.0%)       -4 B  (-0.0%)
plugins                                +39 B  (+0.0%)     +200 B  (+0.0%)
newsletter-post-setup-flow             +39 B  (+0.0%)     -153 B  (-0.2%)
domains                                +26 B  (+0.0%)     +235 B  (+0.1%)
devdocs                                +18 B  (+0.0%)       +4 B  (+0.0%)
bulk-domain-transfer                   +18 B  (+0.7%)      +12 B  (+1.1%)
add-ons                                +18 B  (+0.0%)      +56 B  (+0.1%)
site-purchases                         -13 B  (-0.0%)      +37 B  (+0.0%)
purchases                              -13 B  (-0.0%)      +35 B  (+0.0%)
plugin-bundle-flow                     -13 B  (-0.0%)     +103 B  (+0.3%)
promote-post                           -12 B  (-0.0%)       -3 B  (-0.0%)
posts-custom                           -12 B  (-0.0%)       -3 B  (-0.0%)
posts                                  -12 B  (-0.0%)       -3 B  (-0.0%)
marketplace                            -12 B  (-0.0%)       -3 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 (~216489 bytes added 📈 [gzipped])

name                                                                         parsed_size            gzip_size
async-load-calypso-reader-post-editor                                         +2313204 B     (new)  +636151 B     (new)
async-load-animate-list                                                        -116190 B  (-98.0%)   -37409 B  (-97.9%)
async-load-automattic-global-styles-src-components-global-styles-variations     +53020 B   (+4.0%)   +12892 B   (+3.4%)
async-load-automattic-design-preview                                            +52969 B   (+3.9%)   +13316 B   (+3.5%)
async-load-automattic-help-center                                                 +148 B   (+0.0%)     +110 B   (+0.1%)
async-load-calypso-layout-masterbar-checkout-tsx                                  +131 B   (+0.1%)      +17 B   (+0.0%)
async-load-design                                                                 -116 B   (-0.0%)     +362 B   (+0.1%)
async-load-design-wordpress-components-gallery                                    +110 B   (+0.0%)     -101 B   (-0.1%)
async-load-signup-steps-theme-selection                                            -39 B   (-0.0%)     +443 B   (+1.0%)
async-load-signup-steps-domains                                                    +39 B   (+0.0%)     +196 B   (+0.2%)
async-load-design-blocks                                                           +27 B   (+0.0%)     +274 B   (+0.1%)
async-load-signup-steps-add-ons                                                    +18 B   (+0.0%)      +56 B   (+0.2%)
async-load-signup-steps-woocommerce-install-step-business-info                     -13 B   (-0.0%)      -24 B   (-0.1%)
async-load-quick-language-switcher                                                 -13 B   (-0.0%)      +44 B   (+0.1%)
async-load-design-playground                                                       -13 B   (-0.0%)     +240 B   (+0.1%)
async-load-calypso-components-web-preview-component                                -12 B   (-0.0%)       -3 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.

@matticbot
Copy link
Contributor

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit add/editor-in-reader-behind-flag on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

package.json Outdated Show resolved Hide resolved
config/production.json Outdated Show resolved Hide resolved
client/reader/post-editor/index.jsx Outdated Show resolved Hide resolved
client/data/posts/use-create-new-post.js Outdated Show resolved Hide resolved
client/reader/post-editor/index.jsx Outdated Show resolved Hide resolved
client/data/posts/use-create-new-post.js Outdated Show resolved Hide resolved
@@ -40,6 +40,7 @@
"@automattic/happychat-connection": "workspace:^",
"@automattic/help-center": "workspace:^",
"@automattic/i18n-utils": "workspace:^",
"@automattic/isolated-block-editor": "2.22.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a need to pin it like that, or could we make it less specific with a ^?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ doesn't work, since 0.23.0 updates to React 18 and so is incompatible. But I changed it to 0.22.x.

client/reader/post-editor/index.jsx Show resolved Hide resolved
className="reader-post-editor__publish-button"
isPrimary="true"
onClick={ publishPost }
disabled={ isLoading }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it also be disabled when there are no blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same idea, but found this surprisingly tricky because select( 'core/block-editor' ).getBlocks() seems to only work if run within the scope of IsolatedBlockEditor, rather than within the parent component.

I didn't see an immediate way to pass a function that the button component runs to determine if it's disabled, rather than passing it a predetermined value.

I'll plan to address this in a follow-up, though!

client/reader/following/main.jsx Outdated Show resolved Hide resolved
@creativecoder creativecoder force-pushed the add/editor-in-reader-behind-flag branch from bc19c8c to 715a36d Compare April 26, 2023 16:01
@creativecoder creativecoder force-pushed the add/editor-in-reader-behind-flag branch from 715a36d to 79c2767 Compare June 7, 2023 02:46
@creativecoder creativecoder requested review from a team and worldomonation as code owners June 7, 2023 02:46
@creativecoder creativecoder requested review from razvanpapadopol and removed request for a team June 7, 2023 02:46
@creativecoder creativecoder changed the base branch from trunk to try-react-18 June 7, 2023 02:47
@creativecoder
Copy link
Contributor Author

I've rebased this branch off of try-react-18 and updated isolated-block-editor to the latest version. It's not working yet, but there's less version forcing needed than before: pe7F0s-Vt-p2

@jessie-ross jessie-ross removed the request for review from razvanpapadopol June 7, 2023 12:52
@sirbrillig sirbrillig removed the request for review from a team June 10, 2023 14:31
@noahtallen noahtallen force-pushed the try-react-18 branch 2 times, most recently from f7be757 to 0a0bbb5 Compare June 13, 2023 20:40
@noahtallen noahtallen requested a review from a team as a code owner June 13, 2023 20:40
@bogiii
Copy link
Contributor

bogiii commented Jun 15, 2023

I've reviewed and tested the part of the code that owns the @Automattic/caribou team; it works as expected without regression.

@noahtallen noahtallen force-pushed the try-react-18 branch 3 times, most recently from 03a2c58 to 911478e Compare June 27, 2023 18:32
Base automatically changed from try-react-18 to trunk June 27, 2023 19:15
@creativecoder
Copy link
Contributor Author

I've rebased this branch off of trunk. I wouldn't say the editor works, as there are some UI/style issues, but it builds and you can interact with it without errors.

Copy link
Contributor

@worldomonation worldomonation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure why @Automattic/kitkat was tagged for review, but my concern is that this PR completely breaks Unit Tests.

@noahtallen
Copy link
Contributor

my concern is that this PR completely breaks Unit Tests.

I think this will get resolved after this can be rebased with #78711

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants