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
      }));
    }
Files: 
CommentFileSizeAuthor
#20 core-js-improve-translatability-2052973-20.patch1.22 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 57,615 pass(es).
[ View ]
#16 drupal-improve-translatability-2052973-16.patch1.61 KBMark Carver
PASSED: [[SimpleTest]]: [MySQL] 57,943 pass(es).
[ View ]
#16 interdiff.txt462 bytesMark Carver
#14 drupal-fix-standardize-capitalization-2052973-1.patch1.67 KBMark Carver
PASSED: [[SimpleTest]]: [MySQL] 57,605 pass(es).
[ View ]
#14 interdiff.txt1017 bytesMark Carver
#1 drupal-fix-standardize-capitalization-2052973-1.patch886 bytesMark Carver
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#1 interdiff.txt528 bytesMark Carver

Comments

Status:Active» Needs review
StatusFileSize
new528 bytes
new886 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Fixed bug.

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?

Status:Active» Reviewed & tested by the community

Issue summary:View changes

Updated issue summary.

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".

Status:Needs review» Reviewed & tested by the community

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

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.').

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.

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.

Or 'Tray "@tray" opened'

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.

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

This issue is moving too quick for me :)

Title:Fix "standardize capitalization on actions" in toolbar.jsImprove 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()?

Status:Needs work» Needs review
StatusFileSize
new1017 bytes
new1.67 KB
PASSED: [[SimpleTest]]: [MySQL] 57,605 pass(es).
[ View ]

Addressed @hass' concerns in #8 and #9

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

StatusFileSize
new462 bytes
new1.61 KB
PASSED: [[SimpleTest]]: [MySQL] 57,943 pass(es).
[ View ]

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

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

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.

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.

StatusFileSize
new1.22 KB
PASSED: [[SimpleTest]]: [MySQL] 57,615 pass(es).
[ View ]

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 :)

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.

Status:Reviewed & tested by the community» Fixed

Cool, this looks much better, thanks!

Committed and pushed to 8.x.

Issue tags:-sprint

Removing the sprint tag.

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.

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

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.

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.

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.

Issue summary:View changes

Updated issue summary.