Per #1860434-81: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors:

+ opened: Drupal.t('opened'),
+ closed: Drupal.t('closed'),

Need to be Ucfirst per #421118: [Meta] Standardize capitalization on actions

These are used by accessibility (screen-readers), so it doesn't matter if it's capitalized in the middle (which is needed for easier i18n translations):

onActiveTrayChange: function (model, tray) {
      var relevantTray = (tray === null) ? model.previous('activeTray') : tray;
      var state = (tray === null) ? this.strings.closed : this.strings.opened;
      Drupal.announce(Drupal.t('"@tray" tray @state.', {
        '@tray': relevantTray.querySelector('.toolbar-tray-name').textContent,
        '@state': state
      }));
    }
CommentFileSizeAuthor
#20 core-js-improve-translatability-2052973-20.patch1.22 KBnod_
#16 drupal-improve-translatability-2052973-16.patch1.61 KBmarkhalliwell
#16 interdiff.txt462 bytesmarkhalliwell
#14 drupal-fix-standardize-capitalization-2052973-1.patch1.67 KBmarkhalliwell
#14 interdiff.txt1017 bytesmarkhalliwell
#1 drupal-fix-standardize-capitalization-2052973-1.patch886 bytesmarkhalliwell
#1 interdiff.txt528 bytesmarkhalliwell

Comments

markhalliwell’s picture

markhalliwell
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new interdiff.txt528 bytes
new drupal-fix-standardize-capitalization-2052973-1.patch886 bytes

Fixed bug.

effulgentsia’s picture

effulgentsia commented
Status: Needs review » Active

Regardless of whether the string is shown visually or only read by screen readers, I still don't get what the bug is. Why would we capitalized "opened" and "closed" if it's not the first word of the sentence?

hass’s picture

hass commented
Status: Active » Reviewed & tested by the community
hass’s picture

hass commented
Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

effulgentsia commented
Status: Reviewed & tested by the community » Needs review

Status change unintentional. I guess "needs review" is the appropriate status, but IMO, this should be closed as "works as designed".

hass’s picture

hass commented
Status: Needs review » Reviewed & tested by the community

It's a single word and these need to be ucfirst, too.

effulgentsia’s picture

effulgentsia commented
Status: Reviewed & tested by the community » Needs review

But it's not a single word. It's only ever shown as the @state value in the sentence Drupal.t('"@tray" tray @state.').

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Hm. Per #1860434-82: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors, this doesn't seem correct.

"Foo tray opened." is correct.

"Foo tray Opened." is nonsense.

hass’s picture

hass commented
Status: Needs review » Needs work

Ups. Remove all placeholders and use both strings in their full length and wording. This is a context sensitive bug.

hass’s picture

hass commented

Or 'Tray "@tray" opened'

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

edit: x-post... removed due to irrelevancy

Assigned: Unassigned » jessebeach

The last submitted patch, drupal-fix-standardize-capitalization-2052973-1.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Assigned: jessebeach » Unassigned
Status: Needs review » Needs work

This issue is moving too quick for me :)

effulgentsia’s picture

effulgentsia commented
Title: Fix "standardize capitalization on actions" in toolbar.js » Improve translatability of strings in toolbar.js

Ah, ok. Yeah, I agree with #9. Instead of concatenating within onActiveTrayChange, the opened and closed strings should be full sentences to begin with. Except Tray %tray opened. and Tray %tray closed. would be even better, no? Or are % placeholders no good within Drupal.announce()?

markhalliwell’s picture

markhalliwell
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1017 bytes
new drupal-fix-standardize-capitalization-2052973-1.patch1.67 KB

Addressed @hass' concerns in #8 and #9

hass’s picture

hass commented

Now, with the full sentence strings we do not need the opened/closed strings any longer isn't it?

markhalliwell’s picture

markhalliwell
Primary language English
commented
StatusFileSize
new interdiff.txt462 bytes
new drupal-improve-translatability-2052973-16.patch1.61 KB

Nope, couldn't find any other code that used them. Removed.

effulgentsia’s picture

effulgentsia commented

#16 looks good to me, but I'll let hass RTBC or provide further feedback.

hass’s picture

hass commented

Thanks for working on this. This looks codewise good to me, but I haven't tested it myself. If effulgentsia says it's good to go I trust him to RTBC this.

effulgentsia’s picture

effulgentsia commented
Status: Needs review » Reviewed & tested by the community

I'm not set up for screen reader testing, but the code changes are small and straightforward, so I feel comfortable RTBCing it without the T part.

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-improve-translatability-2052973-20.patch1.22 KB

Don't mind me, a shorter patch that removes duplication. And I know it works because I actually tested it, so leaving RTBC. It appears that there are no messages for screen readers when a tab changes, might have to create a follow-up for that.

Somehow writing JS makes people forget about not duplicating code :)

markhalliwell’s picture

markhalliwell
Primary language English
commented

Thanks @nod_! Beat me to it. Was just about to refactor this. I just quickly did the patches this morning and it wasn't sitting well with me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Cool, this looks much better, thanks!

Committed and pushed to 8.x.

jessebeach’s picture

jessebeach commented
Issue tags: -sprint

Removing the sprint tag.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Fixed » Active

This may have fixed the problem, but it introduced another one.

jessebeach carefully crafted toolbar.js such that options.strings could be overridden via drupalSettings.toolbar.strings. The changes made here broke that. So either the changes made should be moved into options.strings, or we should get rid of options altogether.

hass’s picture

hass commented

Why are you overriding a translatable string this way? There are other ways to override translatable strings... see https://drupal.org/project/stringoverrides

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

If it's wrong, then that should be consistently fixed, rather than only partially as it is now. It may very well be that you're right and the approach taken here (and in other JS files) is wrong! I'm only saying it should be changed consistently and consciously.

effulgentsia’s picture

effulgentsia commented
Status: Active » Fixed

The inconsistency existed before this issue. I opened #2058843: Be consistent with respect to which strings within JS code should be overriddable via drupalSettings for more discussion about that.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

A new issue is good. Even if the inconsistency already existed, it irrefutably was made worse.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.