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 link color and border theme_support #47675

Closed
wants to merge 4 commits into from

Conversation

carolinan
Copy link
Contributor

@carolinan carolinan commented Feb 2, 2023

What?

Updated February 8 2023.

The PR adds theme supports for link color and borders.

Why?

  1. There is no way for themes to enabled these two features without theme.json (Theme.json can not be added to all classic themes without friction).
  2. By using separate theme supports, theme developers can select which feature to support.
  3. The default theme Twenty Twenty-One supported the previous link color theme support which was removed in 5.9. Following that, there have been bug reports on core trac about the link colors in the theme.

I have chosen to add all border settings as one theme support. This is different from how the settings in theme.json works.
In theme.json you enable the color, width, style and radius individually, please comment if you feel these should be separate theme supports.

Closes 41857
Alternative to 47451

Related:
core 57460 Backport: appearance-tools theme_support
core 56487 Bundled themes: opt-in to appearance tools

Testing Instructions

In your WordPress install, make sure that WP_DEBUG_DISPLAY, WP_DEBUG are set to true.
Activate Twenty Twenty-One.
You should see a doing it wrong message about experimental-link-color.
In your code editor, open functions.php in Twenty Twenty-One.
Around line 333, remove
add_theme_support( 'experimental-link-color' );

And add:

add_theme_support( 'link-color' );
add_theme_support( 'border' );

On refresh, the doing it wrong notice should no longer show.

Create a new post.
Add a group block with a paragraph inside. Add a link to the paragraph.
Confirm that the paragraph has a link color setting. Select a custom link color and confirm that it works.
(Palette colors will not work on the front, that is a separate issue)
Add a border to the group, test the different border style, width, and color. (select a custom color with the color picker)
and confirm that it works.

There are known bugs in Twenty Twenty-One that needs to be solved, you do not need to test them as part of this PR,
the main goal is to confirm that the controls are available.

@@ -323,6 +323,26 @@ public static function get_theme_data( $deprecated = array(), $options = array()
if ( current_theme_supports( 'appearance-tools' ) ) {
$theme_support_data['settings']['appearanceTools'] = true;
}

// Allow themes to enable link colors via theme_support.
if ( current_theme_supports( 'link-color' ) ) {
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 considered "being nice" and include the previous name, experimental-link-color in this condition.
What do you think?

@carolinan carolinan marked this pull request as ready for review February 2, 2023 08:08
@carolinan carolinan added Developer Experience Ideas about improving block and theme developer experience [Type] Enhancement A suggestion for improvement. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Needs Decision Needs a decision to be actionable or relevant labels Feb 2, 2023
@carolinan
Copy link
Contributor Author

Only one theme in the theme directory uses the appearance-tools theme support.
https://wpdirectory.net/search/01GRB604V9CJQGP77A4H4V5VPK
But we don't have stats for themes outside the directory.

@Mamaduka
Copy link
Member

Mamaduka commented Feb 3, 2023

An alternative option could be to allow more granular control via the appearance-tools second argument. If no argument is provided, we use the current behavior - appearanceTools: true.

New usage example:

add_theme_support( 'appearance-tools', array(
    'link-color' => true,
    'border'  => true,
) );

What do you think?

@carolinan
Copy link
Contributor Author

We could do that, it would be a smaller code change too.
We would only be removing something that is already broken.

My only remaining concern would be that the theme support would have the same name as the theme.json setting, but not include the same features. This would be confusing. I am not sure documentation would be enough, because there would be an expectation that they match.

@Mamaduka
Copy link
Member

Mamaduka commented Feb 3, 2023

My only remaining concern would be that the theme support would have the same name as the theme.json setting, but not include the same features.

Adding just add_theme_support( 'appearance-tools' ) in the theme will enable the same features as the theme.json equivalent. This way, we can avoid breaking BC. As I understand, this feature flag was introduced in WP 6.1.

Using arguments will allow us only to enable specific features.

@scruffian
Copy link
Contributor

Next steps would be: Revert the appearance-tools addition to core, merge the two new theme_supports instead.

I'm not sure about this. I think we should still keep an easy way to opt in to everything by default, otherwise themers are going to have to go and update their themes every time we add a new support.

@scruffian
Copy link
Contributor

Only one theme in the theme directory uses the appearance-tools theme support.

What about the appearance tools setting in theme.json? I think a lot of themes use that.

@scruffian
Copy link
Contributor

Using arguments will allow us only to enable specific features.

This seems OK. We could even do a similar thing in theme.json:

    "settings": {
        "appearanceTools": {
            'link-color' => true,
            'border'  => true,
        },    
    },

@carolinan
Copy link
Contributor Author

carolinan commented Feb 3, 2023

There seems to be some confusion here still.
If a theme without theme.json adds the add_theme_support( 'appearance-tools' );
The block gap is enabled but the setting does not work.
So unless that is fixed we should not keep it in core.
We should not let them opt-in to something we know doesn't work.

@carolinan
Copy link
Contributor Author

carolinan commented Feb 3, 2023

The support was never added to WP 6.1, only Gutenberg.
It was added to 6.2 alpha about two weeks ago.

@scruffian
Copy link
Contributor

Thanks for clearing that up @carolinan

@carolinan
Copy link
Contributor Author

I was asked to create a new trac ticket for the bug with the appearance tools, and because we have ran out of time for enhancements, the ticket is for reverting without a replacement:
https://core.trac.wordpress.org/ticket/57649

@Mamaduka
Copy link
Member

Mamaduka commented Feb 7, 2023

Thank you, @carolinan!

I think reverting the feature makes sense. It was only introduced in 6.2 alpha and isn't working correctly across the board.

Let's try and ship a more stable appearance-tools 👍

@ndiego
Copy link
Member

ndiego commented Feb 7, 2023

@carolinan given the comments above, can this get punted to 6.3 on the project board?

@carolinan
Copy link
Contributor Author

@carolinan given the comments above, can this get punted to 6.3 on the project board?

Yes please

@carolinan
Copy link
Contributor Author

carolinan commented Feb 8, 2023

Using arguments will allow us only to enable specific features.

This seems OK. We could even do a similar thing in theme.json:

    "settings": {
        "appearanceTools": {
            'link-color' => true,
            'border'  => true,
        },    
    },

@scruffian Could you open a separate issue for that? Still seems like a good idea to me.

@scruffian
Copy link
Contributor

Done here: #47925

@carolinan carolinan requested a review from ndiego as a code owner June 14, 2023 08:29
@carolinan
Copy link
Contributor Author

@WordPress/block-themers Would there be more interest in this if I split the border and link control theme support into two PR's?

@carolinan
Copy link
Contributor Author

Then I will close this PR; You can find the two new PR's linked above.

@carolinan carolinan closed this Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Experience Ideas about improving block and theme developer experience [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Needs Decision Needs a decision to be actionable or relevant [Type] Enhancement A suggestion for improvement.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Consider re-adding link color support for classic themes
4 participants