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

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

Merged
merged 1 commit into from Nov 20, 2019

Conversation

@gziolo
Copy link
Member

gziolo commented Nov 20, 2019

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:
63377650-7e035300-c35e-11e9-9af5-6383f554b1fc

After:
Screen Shot 2019-08-27 at 3 11 01 PM

Copy link
Contributor

youknowriad left a comment

That's cool

@nerrad
nerrad approved these changes Nov 20, 2019
Copy link
Contributor

nerrad left a comment

馃憤 Noice!

@gziolo gziolo merged commit 5a07721 into master Nov 20, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@gziolo gziolo deleted the update/inserter-tip-translation branch Nov 20, 2019
@johnbillion

This comment has been minimized.

Copy link
Member

johnbillion commented Nov 20, 2019

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

This comment has been minimized.

Copy link
Member

johnbillion commented Nov 20, 2019

Would this do it? Untested.

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

This comment has been minimized.

Copy link
Contributor

nerrad commented Nov 20, 2019

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

This comment has been minimized.

Copy link
Contributor

nerrad commented Nov 20, 2019

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

This comment has been minimized.

Copy link
Member

johnbillion commented Nov 20, 2019

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

This comment has been minimized.

Copy link
Contributor

nerrad commented Nov 20, 2019

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

This comment has been minimized.

Copy link
Member Author

gziolo commented Nov 21, 2019

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

This comment has been minimized.

Copy link
Contributor

nerrad commented Nov 21, 2019

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

This comment has been minimized.

Copy link
Member

johnbillion commented Nov 21, 2019

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

This comment has been minimized.

Copy link
Member Author

gziolo commented Dec 5, 2019

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

This comment has been minimized.

Copy link
Member

johnbillion commented Dec 5, 2019

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

@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Dec 5, 2019

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

This comment has been minimized.

Copy link
Member

johnbillion commented Dec 5, 2019

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.

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.