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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gutenberg experiments settings page. #16626

Merged
merged 14 commits into from Aug 6, 2019

Conversation

@tellthemachines
Copy link
Contributor

commented Jul 17, 2019

Description

Addresses #16318 .

  • Added sub-menu page to the Gutenberg admin menu section;
  • Created two settings checkbox fields (Widgets screen and Legacy widget block).

The settings are not actually doing anything yet. As per this comment we still need to discuss what the best strategy is for leaving options enabled or disabled by default.

I'm also not hugely comfortable with the WP Settings API (or with PHP dev in general) so please bear with me here 馃槃 as I have a few questions:

  • What should be added in the @since in function docs when we're not sure what version this is being shipped with?
  • Do checkbox values need any extra validation when being read from/written to the database?
  • This is a new settings page, so semantically it should have an h1 tag. However, add_settings_section generates an h2 and populates it with the section title by default. Is there any way of disabling this? Or, failing that, of adding settings fields without adding a section?
  • Similar problem to the above with add_settings_field: it outputs the field name inside a th, which repeats the field label. Is there any workaround for this?

How has this been tested?

Only checked this locally while building.

Screenshots

Screen Shot 2019-07-17 at 5 13 28 pm

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
lib/experiments-page.php Outdated Show resolved Hide resolved
lib/experiments-page.php Outdated Show resolved Hide resolved
@Soean
Copy link
Member

left a comment

Some strings are not ready for translation.

lib/experiments-page.php Outdated Show resolved Hide resolved
lib/experiments-page.php Outdated Show resolved Hide resolved
lib/experiments-page.php Outdated Show resolved Hide resolved
$value = isset( $options[ $args[1] ] ) ? 1 : 0;
?>
<input type="checkbox" name="<?php echo 'gutenberg-experiments[' . $args[1] . ']'; ?>" id="<?php echo $args[1]; ?>" value="1" <?php checked( 1, $value ); ?> />
<label for="<?php echo $args[1]; ?>">

This comment has been minimized.

Copy link
@talldan

talldan Jul 17, 2019

Contributor

It'd be nice to assign these ($args[0], $args[1]) to variables with informative names at the top of the function. I notice now it's in the comment above the function, but I still think it'd be a nice improvement 馃槃 .

This comment has been minimized.

Copy link
@draganescu

draganescu Jul 17, 2019

Contributor

I've also added this comment below with a more "in depth" explanation :)

This comment has been minimized.

Copy link
@talldan

talldan Jul 17, 2019

Contributor

Good shout 馃憤

*
* @param array $args ( $label, $id ).
*/
function display_experiment_field( $args ) {

This comment has been minimized.

Copy link
@draganescu

draganescu Jul 17, 2019

Contributor

The $args parameter for the add_settings_field function is passed exactly as defined with call_user_func into the callback therefore it would be nice if $args would be a keyed array such as:

add_settings_field(
	'gutenberg-widgets-screen',
	'Widgets Screen',
	'display_experiment_field',
	'gutenberg-experiments',
	'gutenberg_experiments_section',
	array(
		'label' => 'Widgets Screen',
		'id' => 'gutenberg-widgets-screen',
	)
);

because then the code would be easier to read, like:

<label for="<?php echo $args['id']; ?>">
	<?php echo $args['label']; ?>
</label>

Or you could use list to extract named variables out of the args array, for the same readability goal:

function display_experiment_field( $args ) {
	list($label, $id) = $args;

but this approach is breakable if the order in the settings definition will change, so I would go for the named keys option.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Jul 17, 2019

Author Contributor

Oooh good tip, thanks Andrei!

@tellthemachines tellthemachines requested a review from draganescu Jul 18, 2019

@noisysocks

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

Nice work on this! 馃帀

What should be added in the @SInCE in function docs when we're not sure what version this is being shipped with?

This should correspond to the Gutenberg plugin version. The next version will be 6.2.

Do checkbox values need any extra validation when being read from/written to the database?

Nope, the Settings API handles validation and writing to the database 馃檪

This is a new settings page, so semantically it should have an h1 tag. However, add_settings_section generates an h2 and populates it with the section title by default. Is there any way of disabling this? Or, failing that, of adding settings fields without adding a section?

You can pass an empty string ('') to $title in add_settings_section(), though, strangely, this is not documented!

Similar problem to the above with add_settings_field: it outputs the field name inside a th, which repeats the field label. Is there any workaround for this?

It doesn't look like this is possible.

Instead of fighting the API, could we make this screen look more like the other settings pages where groups of checkboxes are grouped together? (cc. @karmatosed)

Screen Shot 2019-07-18 at 14 57 49

Another option is to make the label on the right begin with a verb.

Screen Shot 2019-07-18 at 15 02 59

Both those screenshots are from the Settings > Discussion page in WP Admin.

The settings are not actually doing anything yet. As per this comment we still need to discuss what the best strategy is for leaving options enabled or disabled by default.

I think having them OFF by default makes the most sense. That way, if something that's experimental breaks, the user can take a guess at why (because they explicitly enabled an experiment).

To implement the widgets screen setting, we could wrap our call to add_submenu_page() in an if ( ) that checks that the setting is on.

gutenberg/gutenberg.php

Lines 46 to 53 in e82b7c6

add_submenu_page(
'gutenberg',
__( 'Widgets (beta)', 'gutenberg' ),
__( 'Widgets (beta)', 'gutenberg' ),
'edit_theme_options',
'gutenberg-widgets',
'the_gutenberg_widgets'
);

I'm less certain on how to implement the legacy widget block setting. We might be able to use the allowed_block_types hook, but this only lets us specify a whitelist and not a blacklist. Do you have any ideas, @jorgefilipecosta?

'gutenberg-experiments',
'gutenberg_experiments_section',
array(
'label' => __( 'Legacy Widget Block', 'gutenberg' ),

This comment has been minimized.

Copy link
@swissspidy

swissspidy Jul 18, 2019

Member

That's not really accessible I think. It should be more like "Enable legacy widget block"

'gutenberg-experiments',
'gutenberg_experiments_section',
array(
'label' => __( 'Widgets Screen', 'gutenberg' ),

This comment has been minimized.

Copy link
@swissspidy

swissspidy Jul 18, 2019

Member

That's not really accessible I think. It should be more like "Enable new widgets screen"

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Jul 19, 2019

Author Contributor

Good point! Have changed the labels.

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

Thanks for answering all my questions @noisysocks !

It doesn't look like this is possible.

And thanks for pointing me at the code. I found that by passing a label_for argument to the callback passed to add_settings_field we can get it to output the actual label inside that th 馃榾
This is also not documented. Where is the best place to open a ticket to add this into the docs? It's a really useful thing to know about.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

We discussed this today in the #accessibility weekly slack chat, and agreed that we should rework the labels here a little bit.

We should use "Widgets" as a single group for the two fields, and add more specific labels to the right of each checkbox. Here's a quick mockup:

Untitled-1

@mapk

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

We should use "Widgets" as a single group for the two fields, and add more specific labels to the right of each checkbox.

This feels more consistent with other "Settings" pages and provides clarity. Let's go with that mockup.

@afercia

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

Example of the markup used for the existing "Email me whenever" group under Settings > Discussion (generated with the existing Settings API):

Screenshot 2019-07-19 at 17 34 44

<tr>
    <th scope="row">Email me whenever</th>
    <td>
        <fieldset>
            <legend class="screen-reader-text">Email me whenever</legend>
            <label for="comments_notify">
                <input name="comments_notify" type="checkbox" id="comments_notify" value="1" checked="checked">
                 Anyone posts a comment
             </label><br>

            <label for="moderation_notify">
                <input name="moderation_notify" type="checkbox" id="moderation_notify" value="1" checked="checked">
                A comment is held for moderation
            </label>
        </fieldset>
    </td>
</tr>
@youknowriad
Copy link
Contributor

left a comment

Thanks for the PR. Great page.

Some remarks:

  • I feel widgets screen and legacy widget block work together, so it should be just one option for both
  • We should have an option to show the hidden menu block

I also wonder if we should make the settings work before merging. I think the "editor settings" could work as a way to pass this settings to the editor and use them. Another option could be the use of "inline scripts" somehow. For the widgets screen we could probably just access the setting in the backend before adding the menu item and the customizer panel.

What do you think?

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

I also wonder if we should make the settings work before merging.

Definitely we should make them work; I haven't touched that part yet because I was unsure if they should be disabled by default, particularly the widget ones that are already there, as it might be confusing for users to see them disappear suddenly.

We should have an option to show the hidden menu block

Is the menu block in yet? I thought it was still being worked on. I'm more than happy to set up an option for it 馃槃

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

We should use "Widgets" as a single group for the two fields, and add more specific labels to the right of each checkbox. Here's a quick mockup:

OK so I've been playing around with this and if I add "Widgets" as a title to the settings section, it will be output as a h2 above the checkboxes. Otherwise I can add it as a title to the first of the settings fields, and it will look just like @kjellr 's screenshot but the text will just be inside a th. That doesn't seem right semantically.
Ideally, it should be added to the settings section as a h2 and restyled. Is it possible to override the default wp-admin styles from within the plugin though?

@mapk

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Definitely we should make them work; I haven't touched that part yet because I was unsure if they should be disabled by default, particularly the widget ones that are already there, as it might be confusing for users to see them disappear suddenly.

I believe this is okay. They do say they are (beta) so they can change and will change placement/visibility/etc. Especially now that we're placing these under "Experiments" it makes them feel much less complete.

Is the menu block in yet?

While the menu block is accessible if the user adds the block manually in the Code Editor, it needs some tidying up before we should make this easily available, I think.

Ideally, it should be added to the settings section as a h2 and restyled.

Can we make the markup look like @afercia's example above? Or is there something I'm missing?

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Can we make the markup look like @afercia's example above? Or is there something I'm missing?

I'm afraid @afercia's example isn't using the Settings API: https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/options-discussion.php#L162-L173
There's no way to achieve that markup with the functions the API provides. But if, as per @youknowriad 's comment, we make widgets screen and legacy widget block a single option, that solves the problem for this particular section 馃槃

@mapk

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

But if, as per @youknowriad 's comment, we make widgets screen and legacy widget block a single option, that solves the problem for this particular section

I'm all for this for a quick fix, but as new experiments get added we'll have to eventually solve this issue, correct?

@tellthemachines tellthemachines force-pushed the add/plugin-experiments-settings-page branch from 942a686 to cc89a4e Aug 2, 2019

tellthemachines added some commits Aug 2, 2019

*/
function gutenberg_experiments_editor_settings( $settings ) {
$experiments_exist = get_option( 'gutenberg-experiments' );
return array(

This comment has been minimized.

Copy link
@talldan

talldan Aug 5, 2019

Contributor

I was wondering why so many e2e tests are failing in this branch. I think this is the cause. The $settings being passed in need to be merged with the array of new settings. You should be able to use array_merge.

unstable__bootstrapServerSideBlockDefinitions( { [ name ]: metadata } ); // eslint-disable-line camelcase
}
registerBlockType( name, settings );
registerBlock( block );

This comment has been minimized.

Copy link
@talldan

talldan Aug 5, 2019

Contributor

nitpick, this and the one below could just be forEach( registerBlock )

@mapk

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

I just took another look at this Experiments page again, and it looked good. Here's a screenshot for reference.

Screen Shot 2019-08-05 at 4 15 33 PM

Selecting the checkbox for the widgets screen showed the Widgets screen option in the plugin menu. The Nav block option didn't produce the results I expected because as I went to a post, it appears I lost my ability to add any block to the editor. It's kind of weird and not sure if that's an issue with this PR, or just some artifact due to some rebasing that needs to happen. But I thought to mention it here anyways.

tellthemachines added some commits Aug 6, 2019

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

The Nav block option didn't produce the results I expected because as I went to a post, it appears I lost my ability to add any block to the editor.

Hmm not sure why that would happen, but I've updated a few things and the e2e tests are passing so everything should now be fine 馃槄

await Promise.all( features.map( async ( feature ) => {
await page.waitForSelector( feature );
await page.click( feature );
await page.click( '#submit' );

This comment has been minimized.

Copy link
@talldan

talldan Aug 6, 2019

Contributor

Instead of clicking submit for each feature, that could be done afterwards outside of the loop.

@noisysocks noisysocks added this to the Gutenberg 6.3 milestone Aug 6, 2019

@talldan

talldan approved these changes Aug 6, 2019

Copy link
Contributor

left a comment

This is looking good!

The e2e test util could be improved in a follow up PR.

Would be great to update the those @params and @since version numbers before merging.


await Promise.all( features.map( async ( feature ) => {
await page.waitForSelector( feature );
await page.click( feature );

This comment has been minimized.

Copy link
@talldan

talldan Aug 6, 2019

Contributor

There might be a need to check the existing value in case it already happens to be checked (e.g. by another test, or if there were a single set of tests that toggled the options on and off). There's some similar code in this utility:
https://github.com/WordPress/gutenberg/blob/master/packages/e2e-test-utils/src/toggle-screen-option.js#L17-L20

*
* @param {Array} features Array of {string} selectors of settings to enable. Assumes they can be enabled with one click.
*/
export async function enableExperimentalFeatures( features ) {

This comment has been minimized.

Copy link
@talldan

talldan Aug 6, 2019

Contributor

I wonder if these features need to be disabled again after a test is run. 馃

/**
* Function to register core blocks provided by the block editor.
*
* @param {boolean} enableLegacyWidgets Optional. Editor setting to enable Legacy Widget Block.
* @param {boolean} enableMenu Optional. Editor setting to enable Menu Block.

This comment has been minimized.

Copy link
@talldan

talldan Aug 6, 2019

Contributor

Looks like this was refactored and these two params don't seem to be needed.

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 6, 2019

Member

yes, this should be removed.

/**
* Function to register experimental core blocks depending on editor settings.
*
* @param {Object} settings Editor settings.

This comment has been minimized.

Copy link
@talldan

talldan Aug 6, 2019

Contributor

Would be good to remove the extra white space 馃槃

* registerExperimentalCoreBlocks( settings );
* ```
*/
export const registerExperimentalCoreBlocks = process.env.GUTENBERG_PHASE === 2 ? ( settings ) => {

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 6, 2019

Member

Shouldn't we prefix it with __experimental as this is not meant to be a part of the public API?

See: https://github.com/WordPress/gutenberg/blob/master/docs/contributors/coding-guidelines.md#experimental-and-unstable-apis

tellthemachines added some commits Aug 6, 2019

@noisysocks
Copy link
Member

left a comment

Looks good and tests well! Great work on this @tellthemachines!

@tellthemachines tellthemachines merged commit b4c7054 into master Aug 6, 2019

1 of 4 checks passed

Filter opened Filter opened
Details
Filter opened Filter opened
Details
Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details

@talldan talldan deleted the add/plugin-experiments-settings-page branch Aug 6, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Nice work here. Thanks.

I noticed that the customizer "Widgets Blocks" panel is always visible. This panel should be tied to the "Enable Widgets Screen and Legacy Widget Block" setting.

@tellthemachines

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

I noticed that the customizer "Widgets Blocks" panel is always visible. This panel should be tied to the "Enable Widgets Screen and Legacy Widget Block" setting.

Whoops. I'll fix that in a new PR.

@tellthemachines tellthemachines referenced this pull request Aug 8, 2019
5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.