Remove editor experience when using handheld devices #47

renintw · 2022-09-01T20:45:45Z

Fixes #46

~~The logic can also be placed in the following block to avoid running unnecessary logic, but we have to enqueue the required styles.~~

wporg-gutenberg/source/wp-content/themes/wporg-gutenberg/functions.php

Lines 478 to 487 in 4690151

    
           add_action( 
        
           	'wp_enqueue_scripts', 
        
           	function( $hook ) { 
        
           		// Gutenberg requires the post-locking functions defined within: 
        
           		// See `show_post_locked_dialog` and `get_post_metadata` filters below. 
        
           		include_once ABSPATH . 'wp-admin/includes/post.php'; 
        
           		gutenberg_editor_scripts_and_styles( $hook ); 
        
           	} 
        
           );

Screencast

remove.editor.exp.on.mobile.mov

The font-weight of the heading and the border styles of the button are still off,
that's probably because they're no longer in the editor, so the class name editor-styles-wrapper doesn't exist anymore.
And the button style in the sandbox is different from the one in local env (sandbox has squared black border)

Do we want the styles to be exactly the same as the original ones?

StevenDufresne · 2022-09-01T23:38:46Z

Right, it's missing all the editor-styles-wrapper classes. We need to figure out how to load what is returned from get_block_editor_theme_styles.

tellyworth · 2022-09-02T03:13:20Z

The mobile version might need some slight variations in content. That needn't necessarily be fixed in this PR, but it might be worth opening a new issue after fixing this one to decide what content changes are needed. (Perhaps the "Click around to explore" text)

renintw · 2022-09-05T19:19:48Z

source/wp-content/themes/wporg-gutenberg/functions.php

-			'rest_preload_api_request',
-			array()
-		);
+		if ( wp_is_mobile() ) {


Make this condition inside gutenberg_editor_scripts_and_styles since

We're still rendering Gutenberg editor but in mobile version.

Styles can be shared without duplicating them.

renintw · 2022-09-05T19:22:45Z

source/wp-content/themes/wporg-gutenberg/functions.php

+		if ( wp_is_mobile() ) {
+			$block_editor_css = get_block_editor_theme_styles()[0]['css'];
+			wp_add_inline_style( 'wp-edit-post', $block_editor_css );
+		}


It turns out this worked in the first place, except that it only worked on the sandbox,
locally the newly added styles would somehow be overridden and never show the expected layout.

Does this have to be here? Can it be moved in theif ( wp_is_mobile() ) statement above.

My idea was to put the styles together, but it's also ok to move into the if ( wp_is_mobile() ) statement above.
Done in Fix several issues when loading page.

renintw · 2022-09-05T19:34:15Z

Updated screencast (Styles fixed)

handheld.version.g.mp4

TODO

Fix the shaking when loading the page.
Adjust content to make it conform to the mobile context.

adamwoodnz · 2022-09-05T19:59:01Z

yarn.lock

@@ -2,524 +2,6114 @@
 # yarn lockfile v1


-"@kwsites/file-exists@^1.1.1":


I don't see any changes to package.json, should this have changed?

It turns out that it has the same content as the one in the workspace. I took a further look and found that if we removed the "workspaces": [ "source/wp-content/themes/wporg-gutenberg" ] in source/wp-content/themes/wporg-gutenberg/package.json as well as its corresponding yarn.lock in the same folder and then yarn in source/wp-content/themes/wporg-gutenberg, the yarn.lock would only be generated in the root folder (the one we're looking at), and I'm thinking that this is the way workspace saves space from duplicating packages (by hoisting packages to workspace root). So perhaps we only need one yarn.lock here. Didn't have much experience with the workspace, please let me know if I understand it incorrectly.

Hmm the workspace setup confuses me a bit too, @StevenDufresne do we have a workspace setup for the theme in case one day we need a workspace for a plugin too? I've seen that in other repos

Yeah, it needs some cleanup. The workspace declaration should be removed from the theme and the packages added from the root. We should probably open a new ticket.

Ok. I'll reset the commit and open a new ticket for it. Other than that, do you think this PR is good to go?

It doesn't appear like all the styles are loading properly, a few things I see:

Buttons have different border-radius

The footer has a huge space below it.

Buttons have different border-radius

Which one you're referring to? I saw buttons on the sandbox having 2px border-radius, which is the same as production.

Sandbox

Production

The footer has a huge space below it.

Fixed.

It doesn't appear like all the styles are loading properly

The difference I spot is that the font-weight of h1 and h2 is 600 but not 400 (prod)
and the font-size of some p is smaller than prod.
I've found that the styles have already been imported but were overridden by styles that I can't really tell where they are from. But it can be fixed in a tricky way, I've tested it and it worked, but before really committing it, I'd like to know if there are other ideas for it.

wp_is_mobile() = false wp_is_mobile() = true

I think the best we can do is to enqueue a mobile style sheet that fixes the differences.

Additionally, the <body> tag needs a white background.

I don't see the grey background or the space between the line and the button. (Not sure what makes the differences between our layouts, but could you try again with new commits and see if they are still there, thanks.)
And for the different border-radius of the button, I only saw it locally, not on the sandbox.
Here's the fix for it, now the local would also show the same border-radius as the sandbox and prod.

wp_is_mobile() = true

Sandbox Local Prod

wp_is_mobile() = false

Sandbox Local Prod

StevenDufresne · 2022-09-07T03:21:22Z

Apart from the styles, how do you think we can deal with this copy that appears on the desktop view:

"This page was built with blocks — pieces of content that you can move around. Click around to explore them."

StevenDufresne · 2022-09-07T03:23:23Z

source/wp-content/themes/wporg-gutenberg/functions.php

+		if ( wp_is_mobile() ) {
+			$block_editor_css = get_block_editor_theme_styles()[0]['css'];
+			wp_add_inline_style( 'wp-edit-post', $block_editor_css );
+		}


Does this have to be here? Can it be moved in theif ( wp_is_mobile() ) statement above.

StevenDufresne · 2022-09-07T03:24:28Z

source/wp-content/themes/wporg-gutenberg/functions.php

-			resolve( wp.editPost.initializeEditor( 'editor', "%s", %d, %s, %s ) );
+			$init_script = <<<JS
+	( function() {
+		window._wpLoadBlockEditor = new Promise( function( resolve ) {


Does this need to be loaded on mobile?.. along with the $script block below...

I don't think they are loaded on mobile. (or you're saying that they should be loaded?)

I think it's the format making it hard to read, and the JS part is the one making it unclear (You can see that originally it is aligned to the left side without the indent, and I was thinking that it might be made that way on purpose at first so I didn't add one more indent when including else into the logic). I'll add one first to make the format clearer, and please let me know if I shouldn't do so.

Ah I see, yeah my code collapse function got confused. Thanks. No changes needed.

Would the extra indent affect anything? da207bb If not, I'm tempted to keep it as is as the format looks more readable.

- Shaking - fixed. - Jump to the bottom directly - fixed. - Long loading time - fixed.

renintw · 2022-09-07T21:54:39Z

source/wp-content/themes/wporg-gutenberg/functions.php

+				'wp-edit-post',
+				$block_editor_css . 'body,p{font-size:inherit;} h1,h2,h3,h4,h5,h6{font-weight:inherit;} .wp-block-button__link{border-radius: var(--wp--custom--button--border--radius);}'
+			);
+		} else {


Nothing changed to logic starting from here to the end of the else block.

renintw · 2022-09-07T22:28:18Z

Apart from the styles, how do you think we can deal with this copy that appears on the desktop view:
"This page was built with blocks — pieces of content that you can move around. Click around to explore them."

I'm thinking of only changing Click around to explore them.
Maybe append something like (only available on Desktop) to it?
Or Explore the whole features on Desktop.

StevenDufresne · 2022-09-07T23:01:23Z

Apart from the styles, how do you think we can deal with this copy that appears on the desktop view:
"This page was built with blocks — pieces of content that you can move around. Click around to explore them."

I'm thinking of only changing Click around to explore them. Maybe append something like (only available on Desktop) to it? Or Explore the whole features on Desktop.

I think just removing Click around to explore them. is good enough for now. We can ask for feedback later.

The styles added in this commit primarily make the layout in the local env consistent with the sandbox and the prod. (i.e., the styles on the sandbox and the prod are already aligned)

renintw · 2022-09-12T11:54:24Z

source/wp-content/themes/wporg-gutenberg/style-mobile.css

@@ -24,7 +24,18 @@ h3 {

 .wp-block-button__link {
 	border-radius: var(--wp--custom--button--border--radius);
-	background-color: #32373c;
+	background-color: var(--wp--custom--button--color--background);


The styles added here (L27 - L38) are just for making the layout in the local env consistent with the sandbox and the prod. (i.e., the styles on the sandbox and the prod are already aligned)

StevenDufresne

I think this is close enough for now. I'm not certain how well wp_is_mobile will work in production, but it should be false more than true so its fine.

- Body background-color is set to white. - Link color is set to --wp--custom--color--primary.

renintw requested a review from StevenDufresne September 1, 2022 20:45

renintw self-assigned this Sep 1, 2022

renintw force-pushed the fix/remove-editor-experience-when-using-mobile branch from 4690151 to 22a225b Compare September 5, 2022 19:16

renintw commented Sep 5, 2022

View reviewed changes

adamwoodnz reviewed Sep 5, 2022

View reviewed changes

renintw added 2 commits September 7, 2022 01:14

Remove editor exp when using handheld devices

6abac86

format php

6f79569

renintw force-pushed the fix/remove-editor-experience-when-using-mobile branch from 22a225b to 6f79569 Compare September 6, 2022 17:14

Remove space below the footer

fe47ebb

StevenDufresne reviewed Sep 7, 2022

View reviewed changes

renintw added 4 commits September 8, 2022 00:24

Add indent to make format easier to read

da207bb

Exclude the enqueue of editor scripts

9001eed

Fix font-size / font-heading for mobile version

6504f63

Fix several issues when loading page.

1c6f594

- Shaking - fixed. - Jump to the bottom directly - fixed. - Long loading time - fixed.

renintw force-pushed the fix/remove-editor-experience-when-using-mobile branch from 8ff9f50 to 1c6f594 Compare September 7, 2022 20:06

renintw commented Sep 7, 2022

View reviewed changes

renintw added 2 commits September 8, 2022 07:28

Enqueue custom mobile stylesheet

7f61811

Fix console error by skipping logic not executed

e9da89f

renintw force-pushed the fix/remove-editor-experience-when-using-mobile branch from 23f9881 to e9da89f Compare September 7, 2022 23:31

Adjust text for mobile version

f39b363

renintw requested review from adamwoodnz and StevenDufresne September 7, 2022 23:37

renintw mentioned this pull request Sep 12, 2022

Adjust content for handheld devices #48

Open

renintw force-pushed the fix/remove-editor-experience-when-using-mobile branch from 845ab46 to 3ddcb55 Compare September 12, 2022 11:30

Adjust styles for local env

37fb88c

The styles added in this commit primarily make the layout in the local env consistent with the sandbox and the prod. (i.e., the styles on the sandbox and the prod are already aligned)

renintw force-pushed the fix/remove-editor-experience-when-using-mobile branch from 3ddcb55 to 37fb88c Compare September 12, 2022 11:52

renintw commented Sep 12, 2022

View reviewed changes

StevenDufresne approved these changes Sep 13, 2022

View reviewed changes

Align some styles of mobile with Desktop (prod).

d6293ef

- Body background-color is set to white. - Link color is set to --wp--custom--color--primary.

renintw merged commit bc3ec7e into trunk Sep 13, 2022

renintw deleted the fix/remove-editor-experience-when-using-mobile branch September 13, 2022 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove editor experience when using handheld devices #47

Remove editor experience when using handheld devices #47

renintw commented Sep 1, 2022 •

edited

Loading

StevenDufresne commented Sep 1, 2022

tellyworth commented Sep 2, 2022

renintw Sep 5, 2022

renintw Sep 5, 2022 •

edited

Loading

StevenDufresne Sep 7, 2022

renintw Sep 7, 2022 •

edited

Loading

renintw commented Sep 5, 2022

adamwoodnz Sep 5, 2022

renintw Sep 5, 2022 •

edited

Loading

adamwoodnz Sep 5, 2022

StevenDufresne Sep 6, 2022

renintw Sep 6, 2022

StevenDufresne Sep 6, 2022

renintw Sep 6, 2022 •

edited

Loading

StevenDufresne Sep 7, 2022

StevenDufresne Sep 7, 2022

renintw Sep 7, 2022 •

edited

Loading

StevenDufresne commented Sep 7, 2022

StevenDufresne Sep 7, 2022

StevenDufresne Sep 7, 2022

renintw Sep 7, 2022 •

edited

Loading

StevenDufresne Sep 7, 2022

renintw Sep 7, 2022 •

edited

Loading

renintw Sep 7, 2022

renintw commented Sep 7, 2022 •

edited

Loading

StevenDufresne commented Sep 7, 2022

renintw Sep 12, 2022

StevenDufresne left a comment

	add_action(
	'wp_enqueue_scripts',
	function( $hook ) {
	// Gutenberg requires the post-locking functions defined within:
	// See `show_post_locked_dialog` and `get_post_metadata` filters below.
	include_once ABSPATH . 'wp-admin/includes/post.php';

	gutenberg_editor_scripts_and_styles( $hook );
	}
	);

		@@ -2,524 +2,6114 @@
		# yarn lockfile v1


		"@kwsites/file-exists@^1.1.1":

Remove editor experience when using handheld devices #47

Remove editor experience when using handheld devices #47

Conversation

renintw commented Sep 1, 2022 • edited Loading

Screencast

StevenDufresne commented Sep 1, 2022

tellyworth commented Sep 2, 2022

Choose a reason for hiding this comment

renintw Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renintw Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

renintw commented Sep 5, 2022

Updated screencast (Styles fixed)

TODO

Choose a reason for hiding this comment

renintw Sep 5, 2022 • 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

renintw Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renintw Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

wp_is_mobile() = true

wp_is_mobile() = false

StevenDufresne commented Sep 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renintw Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renintw Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renintw commented Sep 7, 2022 • edited Loading

StevenDufresne commented Sep 7, 2022

Choose a reason for hiding this comment

StevenDufresne left a comment

Choose a reason for hiding this comment

renintw commented Sep 1, 2022 •

edited

Loading

renintw Sep 5, 2022 •

edited

Loading

renintw Sep 7, 2022 •

edited

Loading

renintw Sep 5, 2022 •

edited

Loading

renintw Sep 6, 2022 •

edited

Loading

renintw Sep 7, 2022 •

edited

Loading

renintw Sep 7, 2022 •

edited

Loading

renintw Sep 7, 2022 •

edited

Loading

renintw commented Sep 7, 2022 •

edited

Loading