Add keyboard shortcut styling to "/" in default tip in the inserter modal #17222

enriquesanchez · 2019-08-27T20:28:08Z

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

Before:

After:

talldan

Thanks for working on this @enriquesanchez, this is something that makes complete sense and I'd really like to see merged!

Had a couple of comments, hopefully no major blockers if we can find a solution for the internationalisation.

talldan · 2019-08-28T07:02:56Z

packages/block-editor/src/components/inserter/menu.js

-										'While writing, you can press "/" to quickly insert new blocks.'
+										'While writing, you can press'
+									) }
+									<kbd className="edit-post-keyboard-shortcut-help__shortcut-key">/</kbd>


The class name applied here is from another component, which isn't really something encouraged as it can lead to maintainability issues (i.e. in the future someone changes the other component and doesn't realise this one will be affected).

I think there are three options:

Introduce a generic <Keyboard> react component in the @wordpress/components package that has this styling and can be use across the editor. There would have to be some thought about whether this is something wanted in that package.

Introduce some global styling for the kbd element or a mixin. Not sure about this option as I think it's nice to keep the global styling to a minimum and promote using components instead.

Duplicate some of the styles in tip component.

I think the third one might be preferable. As far as I know there are only two places in the codebase that have kbd styles. When there are three or four it might be time to consider refactoring to a component.

+1 on that third option. Let's just duplicate the styles in this component for now.

If this comes up again we can explore one of the other two options.

Thanks for the feedback @talldan and @kjellr! Agree that option 3 sounds best. I'll push an update.

talldan · 2019-08-28T07:03:00Z

packages/block-editor/src/components/inserter/menu.js

+									) }
+									<kbd className="edit-post-keyboard-shortcut-help__shortcut-key">/</kbd>
+									{ __(
+										' to quickly insert new blocks.'


I'm not completely sure of the best way to internationalise a string that has an HTML tag in it. Maybe using sprintf. I couldn't see any examples in the codebase. I'll ask for some feedback on that.

Feedback on slack is here: https://wordpress.slack.com/archives/C02QB2JS7/p1566976018008100 (requires an account)

Seems as though splitting the string isn't a good idea, but there's no solution yet to internationalising text that has an html element in it. #16374 is a PR that attempts to address it.

Yes. it's very unfortunate that we can't use HTML tags inside translations as of today :(

@nerrad - how far did you get with your implementation for Interpolate component?

Yeah, this definitely seems like something we should have a solution for. In the meantime, is this a blocker for the PR right now?

I can't think of any other way for accomplishing this but with some JS, and I'm not sure that would be desired here. Is it?

Unfortunately I think it is a blocker. 😞

Unless there's a workaround I think it'll hold up the PR.

Sorry for the delay here:

how far did you get with your implementation for Interpolate component?

In the pull I did, the more verbose createInterpolateElement can be extracted from that pull and released as is. It's usable and will solve the problem. I'm mostly waiting on a decision for whether that's a path we want to take initially.

However, the more intuitive jsx-like option with a <Translate> or <Interpolate/> component is not available yet (maybe could be done on a separate pull?).

Other than a brief discussion in a core-javascript slack chat, there hasn't been any feedback on the pull, so I've paused further work until it's clear it's a workable solution and we want to proceed with it.

I opened #18623 to add the same change but using the newly introduced API method.

…nt's class

enriquesanchez · 2019-08-29T18:41:17Z

Closing because this PR is blocked until #16374 is merged.
Thanks everyone for the reviews! 👍

gziolo · 2019-09-03T14:47:33Z

We discussed this PR during the weekly Core JS chat today. @nerrad will work on createInterpolateComponent helper function which should help us unblock this PR. Related comment:
#16374 (comment)

Todo notes after slack chat

extract createInterpolateComponent to its own pull.

consider changing second argument to be an object instead of an array of arrays.

consider removing the necessity for a hasChildren key in the config (instead detecting via regex).

talldan · 2019-12-05T03:01:28Z

Looks like this is now unblocked with the __experimentalCreateInterpolateElement now merged to master.

talldan · 2019-12-05T03:09:10Z

@nerrad Would the correct usage be something like:

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

talldan · 2019-12-05T03:12:04Z

Oh, just noticed this has already been handled in a separate PR, #18623. Ignore my above comments!

gziolo · 2019-12-05T10:12:06Z

😄

There is an ongoing discussion whether <kbd>/<kbd> should be replaced with something else in the translations. I like the idea of having something like:

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

Add keyboard shortcut styling to "/" in default tip

0e90225

enriquesanchez requested review from ajitbohra, chrisvanpatten, ellatrix, gziolo, jorgefilipecosta, talldan and youknowriad as code owners August 27, 2019 20:28

enriquesanchez changed the title ~~Add keyboard shortcut styling to "/" in default tip~~ Add keyboard shortcut styling to "/" in default tip in the inserter modal Aug 27, 2019

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Aug 27, 2019

talldan assigned enriquesanchez Aug 28, 2019

talldan added [Feature] Inserter The main way to insert blocks using the + button in the editing interface [Type] Enhancement A suggestion for improvement. labels Aug 28, 2019

talldan requested changes Aug 28, 2019

View reviewed changes

Duplicate keyboard shortcut styles instead of reusing another compone…

be8df58

…nt's class

enriquesanchez closed this Aug 29, 2019

gziolo deleted the add/keyboard-shortcut-style-to-default-tip branch September 3, 2019 14:46

nerrad mentioned this pull request Sep 9, 2019

New Feature: Introduce __experimentalCreateInterpolateElement #17376

Merged

5 tasks

gziolo mentioned this pull request Nov 20, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add keyboard shortcut styling to "/" in default tip in the inserter modal #17222

Add keyboard shortcut styling to "/" in default tip in the inserter modal #17222

enriquesanchez commented Aug 27, 2019 •

edited

Loading

talldan left a comment •

edited

Loading

talldan Aug 28, 2019

kjellr Aug 28, 2019

enriquesanchez Aug 28, 2019

talldan Aug 28, 2019

talldan Aug 28, 2019

gziolo Aug 28, 2019

kjellr Aug 28, 2019

enriquesanchez Aug 28, 2019

talldan Aug 29, 2019

nerrad Sep 1, 2019 •

edited

Loading

gziolo Nov 20, 2019

enriquesanchez commented Aug 29, 2019

gziolo commented Sep 3, 2019 •

edited

Loading

talldan commented Dec 5, 2019

talldan commented Dec 5, 2019

talldan commented Dec 5, 2019

gziolo commented Dec 5, 2019

Add keyboard shortcut styling to "/" in default tip in the inserter modal #17222

Add keyboard shortcut styling to "/" in default tip in the inserter modal #17222

Conversation

enriquesanchez commented Aug 27, 2019 • edited Loading

talldan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad Sep 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enriquesanchez commented Aug 29, 2019

gziolo commented Sep 3, 2019 • edited Loading

talldan commented Dec 5, 2019

talldan commented Dec 5, 2019

talldan commented Dec 5, 2019

gziolo commented Dec 5, 2019

enriquesanchez commented Aug 27, 2019 •

edited

Loading

talldan left a comment •

edited

Loading

nerrad Sep 1, 2019 •

edited

Loading

gziolo commented Sep 3, 2019 •

edited

Loading