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
      }));
    }
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markhalliwell’s picture

Status: Active » Needs review
FileSize
528 bytes
886 bytes

Fixed bug.

effulgentsia’s picture

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

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

Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

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

Status: Needs review » Reviewed & tested by the community

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

effulgentsia’s picture

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

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

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

Or 'Tray "@tray" opened'

alexpott’s picture

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

Assigned: jessebeach » Unassigned
Status: Needs review » Needs work

This issue is moving too quick for me :)

effulgentsia’s picture

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

Status: Needs work » Needs review
FileSize
1017 bytes
1.67 KB

Addressed @hass' concerns in #8 and #9

hass’s picture

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

markhalliwell’s picture

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

effulgentsia’s picture

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

hass’s picture

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

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

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

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

Status: Reviewed & tested by the community » Fixed

Cool, this looks much better, thanks!

Committed and pushed to 8.x.

jessebeach’s picture

Issue tags: -sprint

Removing the sprint tag.

Wim Leers’s picture

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

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

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

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

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

Issue summary: View changes

Updated issue summary.