Bootstrap the widgets screen design #14612

youknowriad · 2019-03-25T08:46:49Z

This PR bootstraps the widgets screen design:

Saving/fetching... doesn't work
It uses a static list of areas
It cherry-picks Data Module: Support parent/child registries #14369 in order to have separate states for each widget area
It uses the block editor module
It mimics the aria region navigation of the edit post screen

This PR doesn't intent to solve all the issues related to this screen and is intended to land a first dumb screen in order to allow us to parallelize work and start tackling all the related issues.

Testing instructions

Navigate to /wp-admin/admin.php?page=gutenberg-widgets
You should see what ressembles the screenshot above.

youknowriad · 2019-03-25T08:49:53Z

packages/edit-widgets/src/components/header/style.scss

+
+	@include break-medium() {
+		top: $admin-bar-height;
+	}


A lot of these styles to properly style the sidebar and the top bar and the main area are shared with the edit-post module. While I don't mind duplication as it's very related to the screen (those screens are similar) but I think we could extract some reusable mixins here (maybe a follow-up).

jasmussen · 2019-03-25T08:54:36Z

Nice. Just looking at the screenshot, this might be a good opportunity to get work going on the generic appender. It really feels like there should be a "plus" button instead of the text-based appender. Here's a recent mockup I made for the column PR (#14241, CC @getdave) that explores the same:

mapk · 2019-03-25T19:47:31Z

this might be a good opportunity to get work going on the generic appender. It really feels like there should be a "plus" button instead of the text-based appender.

I agree. This was also proposed in the original issue. I think Riad is working his way there most likely. 😉

#13204 (comment)

youknowriad · 2019-03-25T19:51:20Z

yes, actually, there's a lot more to be done here in addition to the appender change. The idea of this PR is to provide the basis in order to allow us to fix all these issues in multiple parallel PRs.

Personally, the only blocker I see in this PR is #14369. It's cherry-picked here but should be solved first and then we can merge this and iterate.

aduth

Code looks good, particularly mindful of scaffolding nature of the pull request 👍 I'll give it a more thorough shake tomorrow, assuming via #14369 (comment) there's more work to be done with the parent/child store implementation.

lib/widgets-page.php

aduth · 2019-03-26T00:54:16Z

packages/edit-widgets/src/components/sidebar/style.scss

+	border-left: $border-width solid $light-gray-500;
+	background: $white;
+	color: $dark-gray-500;
+	height: 100vh;


Is height necessary if top, right, bottom are zero'd ?

I don't really know, this is copied from the edit-post but I can test.

This looks like an explicit change done for mobile here #1691

In my testing it doesn't impact to remove but given how fragile these things are, I prefer to leave and defer to @jasmussen

aduth · 2019-03-26T00:56:38Z

packages/edit-widgets/src/components/widget-area/index.js

+} from '@wordpress/data';
+
+function WidgetArea( { title, initialOpen, registry } ) {
+	// Disable reason, this rule conflicts with the React hooks rule (no conditionals)


Interesting conflict for hooks 😬 I guess we could make some assumptions around the "use" prefix for hooks as an exception. Not sure how to express that, since the rule otherwise applies to functions generally, not specific to React components.

(Should be filed as a separate issue; I can help)

Should we just remove the rule? It doesn't bring me joy 😛

aduth · 2019-03-26T00:58:17Z

packages/edit-widgets/src/components/widget-area/index.js

+	const [ blocks, updateBlocks ] = useState( [] );
+	const [ subRegistry, updateRegistry ] = useState( null );
+	useEffect( () => {
+		// TODO: This behavior should be embedded in the BlockEditorProvider


Why not do it? Is it a conflict with how we use just a single provider in the post editor screen? If so, won't that always be the case?

This requires #14369 so I was just waiting for it.

I'm going to open a PR for this separately.

Done in #14678

packages/edit-widgets/src/style.scss

noisysocks

Tested and it looks like a good stub 👍 — will be useful for #14251!

packages/data/src/registry.js

noisysocks · 2019-03-29T00:26:01Z

packages/edit-widgets/src/components/widget-area/index.js

+} from '@wordpress/data';
+
+function WidgetArea( { title, initialOpen, registry } ) {
+	// Disable reason, this rule conflicts with the React hooks rule (no conditionals)


Should we just remove the rule? It doesn't bring me joy 😛

noisysocks · 2019-03-29T00:37:35Z

packages/edit-widgets/src/components/layout/index.js

+import WidgetArea from '../widget-area';
+
+function Layout() {
+	const areas = [


Time for our favourite past time... ✨ naming things! ✨

It looks like WordPress calls the part of the page where a group of widgets appears a "sidebar" in the code but a "Widget Area" in the documentation. Which term should we use in Gutenberg?

I am thinking it would be confusing, after Phase 2 is merged into trunk, to have both wp.data.select( 'core/edit-widgets' ).getWidgetAreas() and register_sidebar().

(Nothing to do with this PR—just trying to get ahead of the terminology here.)

Yeah, I don't really know. I don't like "sidebars" personally as it's not really what these things are meant for but I can be on board with it for consistency. @aduth our naming expert to the rescue? :P

The help text in the current widgets screen also includes some references to this concept as a "widget area", so there's certainly precedent for both cases. I assume it's likely a matter of historical legacy on the "sidebar" naming.

Phase 2 could be a good opportunity to push to adopt the "area" naming, and move away from considering these as constrained to sidebars. Consistency with existing function naming is worth considering, but I also wonder if it's not out-of-the-question to consider a soft-deprecation of existing register_sidebar functions in favor of newly-introduced equivalent register_widget_area, etc. functions? It would need to be a conscious and agreed-upon effort, but I could definitely get behind "area" naming if that were an option.

In any case, I also agree that it needn't block merging this, especially as they're not exposed as part of a public interface (aside from the one class name).

There was some conversation about this in the customizer issue as well. I went with "Block Areas" in the prototype.

youknowriad · 2019-03-29T08:22:21Z

I think this is not ready to land to be able to work in parallel after.

aduth · 2019-04-01T20:24:06Z

packages/edit-widgets/src/components/header/style.scss

+		top: $admin-bar-height;
+	}
+}
+@include editor-left(".edit-widgets-header");


Possible future task: Is there sense in making this specific to "editor"? Or is it more about how the UI interacts with the admin sidebar more generally?

aduth · 2019-04-01T20:47:50Z

packages/edit-widgets/src/components/layout/index.js

+import WidgetArea from '../widget-area';
+
+function Layout() {
+	const areas = [


The help text in the current widgets screen also includes some references to this concept as a "widget area", so there's certainly precedent for both cases. I assume it's likely a matter of historical legacy on the "sidebar" naming.

Phase 2 could be a good opportunity to push to adopt the "area" naming, and move away from considering these as constrained to sidebars. Consistency with existing function naming is worth considering, but I also wonder if it's not out-of-the-question to consider a soft-deprecation of existing register_sidebar functions in favor of newly-introduced equivalent register_widget_area, etc. functions? It would need to be a conscious and agreed-upon effort, but I could definitely get behind "area" naming if that were an option.

In any case, I also agree that it needn't block merging this, especially as they're not exposed as part of a public interface (aside from the one class name).

aduth · 2019-04-01T20:53:00Z

lib/widgets-page.php

@@ -29,6 +29,10 @@ function gutenberg_widgets_init( $hook ) {
 			return;
 	}

+	wp_add_inline_script(
+		'wp-edit-widgets',
+		'wp.editWidgets.initialize( "widgets-editor" )'


This should include a terminating semi-colon. Otherwise it may result in other wp_add_inline_script for wp-edit-widgets being treated as a continuation of this code, e.g. an IIFE would result in "VM662:2 Uncaught TypeError: wp.editWidgets.initialize(...) is not a function".

aduth · 2019-04-01T21:00:39Z

I observe local changes to package-lock.json as a result of the edit-widgets dependency changes. Those should be committed.

aduth · 2019-04-01T21:01:35Z

I think this is not ready to land to be able to work in parallel after.

I assume you meant (and had previously read this as) "now ready"... which has a very different meaning from a literal interpretation of the text 😄

mapk · 2019-04-05T21:25:28Z

I can't wait to see continued progress on this! It's looking good.
Since there's a "Needs Design Feedback" label here, I'll provide some. I'm fully aware this isn't really near completion, so take this with a grain of salt.

1. The background grey
I like this blueish-grey color, but maybe we should stick with the default grey background color provided in WordPress? background-color: #f1f1f1
Unless, of course, this grey is more consistent with Gutenberg greys.

2. Block widths
The width of the blocks within each Block Area appear a bit narrow.

3. Some blocks should be excluded
I noticed I can add a "Page Break" block which doesn't seem to make sense in a siderbar or footer.

4. Block toolbar
There should be more padding above the first block in the area. Right now the block toolbar extends into the accordion title which doesn't seem right.

5. Block appender
Just noting the block appender should be included in each area.

Let's remove the design feedback label for now since this is also being further discussed in the RFC. Once there's more to review, we can add the label back.

youknowriad · 2019-04-08T11:45:07Z

Thanks for the great feedback @mapk

Updated
That's because the default block editor contain padding to show the block movers and there's probably some extra unnecessary padding there.
Definitely, I think there needs to be APIs/rules to show/exclude blocks from post content editor and from widget editors. I think this deserves a big discussion to define it properly. For example, maybe all blocks depending on the wp-editor script could be considered post-specific?
I noticed that the toolbar don't show up consistently. Not sure why yet but it deserves a follow-up

I also noticed that the block movers are not showing-up (not sure why yet).

I'm going to merge this PR. there's a lot of work remaining here, but this should give us the basis from which we will be able to iterate all together.

youknowriad requested review from aduth, chrisvanpatten, gziolo, nerrad, noisysocks and talldan as code owners March 25, 2019 08:46

youknowriad self-assigned this Mar 25, 2019

youknowriad added Needs Design Feedback Needs general design feedback. Needs Accessibility Feedback Need input from accessibility [Feature] Widgets Screen The block-based screen that replaced widgets.php. labels Mar 25, 2019

youknowriad commented Mar 25, 2019

View reviewed changes

youknowriad requested review from mapk, karmatosed and jasmussen March 25, 2019 08:51

aduth reviewed Mar 26, 2019

View reviewed changes

youknowriad mentioned this pull request Mar 28, 2019

Support storing blocks in sidebars #14251

Closed

7 tasks

youknowriad force-pushed the add/widgets-scree-ui branch from 83958e9 to 5f2db99 Compare March 28, 2019 08:37

youknowriad mentioned this pull request Mar 28, 2019

Add the React hooks eslint to our default eslint config #14676

Closed

noisysocks reviewed Mar 29, 2019

View reviewed changes

youknowriad added 5 commits March 29, 2019 09:10

Support parent/child registries

ce4cf5c

Bootstrap the widgets screen design

b5fdae6

Fix phpcs issues

5f3f9c5

Initialize using an id

086ae8f

Remove subregistry as it's baked in now

158d62e

youknowriad force-pushed the add/widgets-scree-ui branch from e955150 to 158d62e Compare March 29, 2019 08:18

Remove duplicate code after rebase

b64efca

aduth approved these changes Apr 1, 2019

View reviewed changes

Changes per review

9b26843

noisysocks mentioned this pull request Apr 4, 2019

Add Blocks in Widget Areas RFC #14812

Closed

mapk removed the Needs Design Feedback Needs general design feedback. label Apr 5, 2019

Update the background color to match Core

808b5d1

youknowriad merged commit 1df3021 into master Apr 8, 2019

youknowriad deleted the add/widgets-scree-ui branch April 8, 2019 11:48

youknowriad added this to the 5.5 (Gutenberg) milestone Apr 12, 2019

This was referenced Apr 15, 2019

Bump version number to 5.5.0-rc.1 #14981

Merged

Bump/5.5 #15011

Closed

Release/5.5 #15012

Closed

Bump plugin version to 5.5 #15013

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bootstrap the widgets screen design #14612

Bootstrap the widgets screen design #14612

youknowriad commented Mar 25, 2019

youknowriad Mar 25, 2019

jasmussen commented Mar 25, 2019

mapk commented Mar 25, 2019

youknowriad commented Mar 25, 2019

aduth left a comment

aduth Mar 26, 2019

youknowriad Mar 26, 2019

youknowriad Mar 28, 2019

aduth Mar 26, 2019

noisysocks Mar 29, 2019

aduth Mar 26, 2019

youknowriad Mar 26, 2019

youknowriad Mar 28, 2019

youknowriad Mar 28, 2019

noisysocks left a comment

noisysocks Mar 29, 2019

noisysocks Mar 29, 2019 •

edited

Loading

youknowriad Mar 29, 2019

aduth Apr 1, 2019

mapk Apr 5, 2019

youknowriad commented Mar 29, 2019

aduth Apr 1, 2019

aduth Apr 1, 2019

aduth Apr 1, 2019

aduth commented Apr 1, 2019

aduth commented Apr 1, 2019

mapk commented Apr 5, 2019

youknowriad commented Apr 8, 2019

Bootstrap the widgets screen design #14612

Bootstrap the widgets screen design #14612

Conversation

youknowriad commented Mar 25, 2019

Choose a reason for hiding this comment

jasmussen commented Mar 25, 2019

mapk commented Mar 25, 2019

youknowriad commented Mar 25, 2019

aduth 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

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

noisysocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisysocks Mar 29, 2019 • 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

youknowriad commented Mar 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Apr 1, 2019

aduth commented Apr 1, 2019

mapk commented Apr 5, 2019

youknowriad commented Apr 8, 2019

noisysocks Mar 29, 2019 •

edited

Loading