Added Settings UI #66

akshitsethi · 2022-04-25T10:26:44Z

Closes #38, #54

Description of the Change

This PR adds UI for Plugin settings to configure the post_types supported by the plugin. Currently, it is being done with the help of a filter hook. With this change, users will be able to visually select the post_types they wish to be configured with the plugin.

Also, missing yoast/phpunit-polyfills package has been added to enable running of unit tests.

To-do:

Polishing PR
Configure filter to use post_types value from DB to configure the plugin

Possible Drawbacks

None that I can think of.

Verification Process

To check the working of the Settings panel, follow the steps below:

Click "Convert to Blocks" under the "Settings" menu.
On the Settings page, de-select one of the default post types, i.e. either page or post.
Navigate to the de-selected item's menu (Page, Post or CPT) and verify that it is no longer supported by the plugin.

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
All new and existing tests passed.

Changelog Entry

Added - Settings UI for managing supported post_types

dinhtungdu

@akshitsethi thanks so much for the PR! Testing this PR I found some issues:

Activating the plugin for the first time, the Post and Page are unchecked in the setting page, while they're enabled by default. Pages and posts are still converted to blocks automatically. This can make users confused.
This doesn't fully resolve Add settings UI to opt CPTs into convert-to-blocks support #54 because the custom post types with editor support are still forced to use the classic editor.

dinhtungdu · 2022-05-11T04:50:45Z

includes/ConvertToBlocks/Settings.php

+		add_action( 'admin_init', [ $this, 'register_section' ], 10 );
+		add_action( 'admin_init', [ $this, 'register_fields' ], 20 );
+
+		add_filter( 'post_type_supports_convert_to_blocks', [ $this, 'supported_post_types' ], PHP_INT_MAX, 2 );


Using PHP_INT_MAX makes it very hard for plugins and/or themes to use this filter. I think even using the default priority is enough.

@dinhtungdu I totally agree. I was myself having second thoughts about this. I'll correct this.

akshitsethi · 2022-05-11T11:49:33Z

@dinhtungdu

Activating the plugin for the first time, the Post and Page are unchecked in the setting page, while they're enabled by default. Pages and posts are still converted to blocks automatically. This can make users confused.

There are certain scenarios here which I would like a bit more clarity on:

Scenario 1

Post and pages are enabled by default. What if a user wants to disable the functionality on pages but keep it on for posts?
Should we factor this in the plugin or keep it enabled by default for both (without allowing to de-activate).

Another point to consider here is that if we do keep the settings enabled by default for the user, then on a blog without CPTs, we won't have any setting to configure.

Scenario 2

An existing user has configured the plugin using the filter but if we update plugin settings in DB enabling post and pages options by default, it might break the existing filter they have in place. One way to tackle this is to ensure that our filter runs first and then it can be overriden later.

Do you think updating DB option on plugin activation should be done?

dinhtungdu · 2022-05-11T15:40:26Z

Post and pages are enabled by default. What if a user wants to disable the functionality on pages but keep it on for posts?
Should we factor this in the plugin or keep it enabled by default for both (without allowing to de-activate).

What I mean is we should respect the default setting. If users activate the plugin for the first time, Post and Page should be checked on the setting page (there is no data in the database at that point). If user have another CPTs, those post type should be unchecked by default.

An existing user has configured the plugin using the filter but if we update plugin settings in DB enabling post and pages options by default, it might break the existing filter they have in place.

The custom filter added by users should have the highest priority and should disable the setting section. We also should display a message let users know that there is an active filter controlling the post type.

Do you think updating DB option on plugin activation should be done?

Because of the above points, I don't think we need a DB updater here.

dinhtungdu · 2022-05-13T05:10:41Z

includes/ConvertToBlocks/Settings.php

+	public function field_post_types() {
+		$post_types  = get_option(
+			sprintf( '%s_post_types', CONVERT_TO_BLOCKS_SLUG ),
+			apply_filters( 'convert_to_blocks_default_post_types', CONVERT_TO_BLOCKS_DEFAULT_POST_TYPES )


We should avoid using the same filter in different places when possible.

For this case, we can define the $cointainer property for this class, and get the default post types using `$this->container->get_default_post_types(), see this for more details.

By doing so, we don't need the CONVERT_TO_BLOCKS_DEFAULT_POST_TYPES constant anymore.

dinhtungdu · 2022-05-13T05:13:32Z

includes/ConvertToBlocks/Settings.php

+				<?php
+				printf(
+					/* translators: %1$s: link to switch to settings panel, %2$s: closing anchor tag */
+					esc_html__( 'A filter hook (post_type_supports_convert_to_blocks) is active which can lead to undesirable outcome. Kindly remove it and configure settings via the %1$sSettings Panel%2$s.', 'convert-to-blocks' ),


Kindly remove it and configure settings via the %1$sSettings Panel%2$s.

~~The notice is only displayed on the Convert to Blocks setting page, so we don't need this IMO.~~

Edit: the notice is displayed for every admin page. I think we should limit it to the post/page list, plugin page, and the Convert to Blocks setting pages.

Also, a filter can be used for advanced conditions like preventing conversion for posts belonging to a category. So IMHO, we just need to notify users that one or more filters to post_type_supports_convert_to_blocks are being used, without any call to action.

jeffpaul · 2022-09-12T17:42:38Z

@akshitsethi are you able to work on the feedback items here or would it be best to grab another person to help wrap up this PR?

Sidsector9 · 2022-10-17T09:08:02Z

@jeffpaul, @akshitsethi said this needs a little more work and he's happy to have someone from the OSP team to takeover.

cadic · 2022-10-17T10:22:51Z

@Sidsector9 @jeffpaul will work on this during my OSP week.

jeffpaul · 2022-11-14T19:53:49Z

@cadic did you manage to make progress on the updates here locally and can push up to the PR?

jeffpaul · 2022-12-19T17:40:40Z

@Sidsector9 @cadic checking to see if either of you are able to help get this updated and off to code review in January perhaps?

…ure/58-settings-ui

Sidsector9

LGTM 👍

I added few fixes for workflows for them to pass, downgraded some NPM dependencies as npm run build broke.

@jeffpaul should we bump the minimum PHP version of this plugin from 7.0 to 7.4?

jeffpaul · 2023-01-05T14:46:10Z

@Sidsector9 yes, fine to bump PHP min to 7.4

…ure/58-settings-ui

akshitsethi added 2 commits April 25, 2022 15:50

Added Settings UI for the plugin utilising Settings API

57d4d1f

Add phpunit-polyfills to enable running unit tests

7f4c3ec

akshitsethi force-pushed the feature/58-settings-ui branch from d9b532a to 7f4c3ec Compare April 25, 2022 11:25

Improvements to variables and checkbox rendering

5441438

akshitsethi changed the title ~~Added Settings UI for the plugin utilising Settings API~~ Added Settings UI Apr 25, 2022

jeffpaul added this to the 1.1.0 milestone Apr 26, 2022

jeffpaul assigned akshitsethi Apr 26, 2022

jeffpaul linked an issue Apr 26, 2022 that may be closed by this pull request

Add settings UI to opt CPTs into convert-to-blocks support #54

Closed

akshitsethi added 2 commits May 10, 2022 13:40

Add filter to configure supported post types

b8ed9f3

Move filter to the end

ef4e2cb

akshitsethi marked this pull request as ready for review May 10, 2022 08:32

Fix nitpicks

e05f49b

akshitsethi requested a review from jeffpaul May 10, 2022 08:43

jeffpaul requested review from a team and dinhtungdu and removed request for jeffpaul and a team May 10, 2022 18:14

dinhtungdu reviewed May 11, 2022

View reviewed changes

Apply suggested PR changes

d33e248

jeffpaul requested a review from dinhtungdu May 12, 2022 14:33

dinhtungdu reviewed May 13, 2022

View reviewed changes

dkotter modified the milestones: 1.1.0, 1.2.0 Jul 27, 2022

jeffpaul requested review from a team and Sidsector9 and removed request for a team October 10, 2022 15:55

cadic assigned cadic and unassigned akshitsethi Oct 17, 2022

Merge branch 'develop' of github.com:10up/convert-to-blocks into feat…

83d5399

…ure/58-settings-ui

Sidsector9 force-pushed the feature/58-settings-ui branch from 25b8a49 to 83d5399 Compare January 5, 2023 11:45

Sidsector9 added 4 commits January 5, 2023 17:18

fix phpcs workflow

e5c4889

fix phpunit workflow

4b5ffef

remove deprecated workflow action

f857e27

add code review changes

4b94306

Sidsector9 previously approved these changes Jan 5, 2023

View reviewed changes

Sidsector9 dismissed their stale review via 1b016d4 January 5, 2023 14:52

Sidsector9 force-pushed the feature/58-settings-ui branch from a7241f2 to 4b94306 Compare January 5, 2023 14:57

Merge branch 'develop' of github.com:10up/convert-to-blocks into feat…

1fc2558

…ure/58-settings-ui

Sidsector9 approved these changes Jan 5, 2023

View reviewed changes

jeffpaul merged commit cb4b48e into develop Apr 7, 2023

jeffpaul deleted the feature/58-settings-ui branch April 7, 2023 14:32

jeffpaul mentioned this pull request Jun 8, 2023

Release/1.2.0 #111

Merged

4 tasks

This was referenced Jun 16, 2023

Revert "Post type setting field added" #112

Merged

upkeep/66: change notice from success to warning #114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Settings UI #66

Added Settings UI #66

akshitsethi commented Apr 25, 2022 •

edited

Loading

dinhtungdu left a comment

dinhtungdu May 11, 2022

akshitsethi May 11, 2022

akshitsethi commented May 11, 2022

dinhtungdu commented May 11, 2022

dinhtungdu May 13, 2022

dinhtungdu May 13, 2022 •

edited

Loading

dinhtungdu May 13, 2022

jeffpaul commented Sep 12, 2022

Sidsector9 commented Oct 17, 2022

cadic commented Oct 17, 2022

jeffpaul commented Nov 14, 2022

jeffpaul commented Dec 19, 2022

Sidsector9 left a comment

jeffpaul commented Jan 5, 2023

Added Settings UI #66

Added Settings UI #66

Conversation

akshitsethi commented Apr 25, 2022 • edited Loading

Description of the Change

Possible Drawbacks

Verification Process

Checklist:

Changelog Entry

dinhtungdu left a comment

Choose a reason for hiding this comment

dinhtungdu May 11, 2022

Choose a reason for hiding this comment

akshitsethi May 11, 2022

Choose a reason for hiding this comment

akshitsethi commented May 11, 2022

Scenario 1

Scenario 2

dinhtungdu commented May 11, 2022

dinhtungdu May 13, 2022

Choose a reason for hiding this comment

dinhtungdu May 13, 2022 • edited Loading

Choose a reason for hiding this comment

dinhtungdu May 13, 2022

Choose a reason for hiding this comment

jeffpaul commented Sep 12, 2022

Sidsector9 commented Oct 17, 2022

cadic commented Oct 17, 2022

jeffpaul commented Nov 14, 2022

jeffpaul commented Dec 19, 2022

Sidsector9 left a comment

Choose a reason for hiding this comment

jeffpaul commented Jan 5, 2023

akshitsethi commented Apr 25, 2022 •

edited

Loading

dinhtungdu May 13, 2022 •

edited

Loading