Inserter: Add keyboard shortcut styling to "/" in the default tip

[gziolo]

Description

Fixes: #17111.

Alternative approach to #17222. Props to @enriquesanchez for initial efforts.

It uses new experimental API added in #17376:
__experimentalCreateInterpolateElement
Props to @nerrad 😍

This PR adds keyboard shortcut styling to the "/" in the default tip in the inserter modal.

Before:

After:

[youknowriad]

That's cool

[nerrad]

👍 Noice!

[johnbillion]

The preferred string here would not contain any HTML. Example:

sprintf(
  'While writing, you can press %s to quickly insert new blocks.',
  '<kbd>/</kbd>
)

Is this possible with the current implementation?

[johnbillion]

Would this do it? Untested.

{ __experimentalCreateInterpolateElement(
    sprintf(
        __( 'While writing, you can press %s to quickly insert new blocks.' ),
        '<kbd>/</kbd>'
    ),
    { kbd: <kbd /> }
) }

[nerrad]

The preferred string here would not contain any HTML.

While generally, I agree with you, historically the WordPress core project has accepted (whitelisted tags?) html in translation strings. Granted, a cursory review of developer documentation doesn't seem to surface anything explicit about this but maybe it's time to review and arrive at some conventions for what WordPress core (as opposed to plugins/themes) will or won't accept in i18n strings? With the inclusion of this function, maybe that's something due for a wider conversation sooner vs later?

[nerrad]

As an aside, I think @johnbillion 's suggestion here is preferred anyways because it protects / as something that shouldn't be translated (I probably should have caught that in my review 😦 ).

[johnbillion]

I don't think there's anything official but core is definitely trying where possible to avoid introducing HTML in new strings, and removing existing HTML from strings (https://core.trac.wordpress.org/search?ticket=on&q=html%20tags%20in%20strings).

Protecting constants from translation is a benefit as you mentioned. It also helps reduce the chance that a translation alters or breaks markup as an extra layer on top of the protections in place on translate.w.org.

[nerrad]

I don't think there's anything official but core is definitely trying where possible to avoid introducing HTML in new strings, and removing existing HTML from strings

I applaud this effort (for all the reasons you've mentioned) and I think it should be surfaced more widely via a dev note or WP core discussion. This is the first I've heard of the effort (not saying it was anyone's responsibility to tell me lol), and I imagine there are others in the space not super aware either. It sounds like something that would be good to formalize in the docs?

The good news is the current function here supports this (via using sprintf as you pointed out), so assuming this becomes a new convention, it can be followed. I imagine this is going to surface in more reviews too though so should probably be addressed. I'll bring this up in the core-editor meeting today.

[gziolo]

The good news is the current function here supports this (via using sprintf as you pointed out), so assuming this becomes a new convention, it can be followed. I imagine this is going to surface in more reviews too though so should probably be addressed. I'll bring this up in the core-editor meeting today.

I'm happy to open a follow-up PR to change the current implementation. This API is marked experimental for a reason :) Did you discuss what would be the best way to approach such translations? What if there are more than one tokens to replace?

[nerrad]

Did you discuss what would be the best way to approach such translations?

Nope the meeting time got eaten up before we got to the issue (I had also added it as a comment on the agenda post but it got missed I think).

What if there are more than one tokens to replace?

I'm still uncertain of what approach we should be recommending or taking because I've heard conflicting things from various core folks over the years regarding using html tags in i18n (specifically in WP core) and with the lack of any official stance documented it's hard to know exactly what to do.

Further, #9846 was basically birthed out of a described need for using strings with tags interpolated in them. I also find it odd that there's at least a few instances of trac tickets in the search results @johnbillion pointed to that are adding tags into i18n strings (not the other way around as he mentioned - although I do see a few of those as well): one of them here. In fact, as I review that search, it seems that there are still cases where html tags are used in strings but there seems to be a unspoken rule being followed regarding when a tag is allowed or not.

So, long story short, I think there needs to be something formalized so there's clarity in what "rules" contributors should follow here (cc @swissspidy , @ocean90 ) for strings landing in WordPress core.

I'm happy to open a follow-up PR to change the current implementation.

Ya we probably should modify this one specifically because it's a keyboard symbol that we wouldn't want translators modifying. You can add the appropriate translators comment string as well.

[johnbillion]

For clarity, the general approach in core in recent years has been to remove HTML from strings, but where HTML does need to be present in strings then it should be plain HTML and not use the opening-and-closing-tag-placeholder approach. That's what changeset 45334 was doing.

Agreed that more clarity would help.

[gziolo]

Re-sharing my comment from the parent issue.

I like the idea of having something like:

createInterpolateElement(
    __( 'While writing, you can press <SlashKey /> to quickly insert new blocks' ),
    { SlashKey: <kbd>/</kbd> }
);

What do you think?

[johnbillion]

What's the benefit of that over the more common %s placeholder format?

[gziolo]

It gives the translator a better context and you don't need to use sprintf to replace it. It also scales well if you want to use more tokens and even repeat them.

createInterpolateElement(
    __(
        'While writing, you can press <SlashKey /> to quickly insert new blocks. ' +
        '<SlashKey /> is really helpful. <EnterKey /> can be useful as well...'
    ),
    {
        EnterKey: <kbd>Enter</kbd>,
        SlashKey: <kbd>/</kbd>,
    }
);

[johnbillion]

Good points, but all of that is available with the existing i18n infrastructure.

CreateInterpolateElement(
    sprintf(
        /* translators: 1: Key to insert blocks. 2: Another key */
        __( 'While writing, you can press %1$s to quickly insert new blocks. %1$s is really helpful. %2$s can be useful as well...' ),
        '<kbd>/</kbd>',
        '<kbd>Enter</kbd>'
    ),
    { kbd: <kbd /> }
)

Your example may be syntactically more appealing for developers but there's real value in standardising string formats for i18n. If you're a translator you're going to wonder why some strings use %s for placeholders and some contain custom self-closing HTML elements that turn out to also be placeholders.