Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
+ 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
}));
}
Comment | File | Size | Author |
---|---|---|---|
#20 | core-js-improve-translatability-2052973-20.patch | 1.22 KB | nod_ |
#16 | drupal-improve-translatability-2052973-16.patch | 1.61 KB | markhalliwell |
#16 | interdiff.txt | 462 bytes | markhalliwell |
#14 | drupal-fix-standardize-capitalization-2052973-1.patch | 1.67 KB | markhalliwell |
#14 | interdiff.txt | 1017 bytes | markhalliwell |
Comments
Comment #1
markhalliwellFixed bug.
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedRegardless 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?
Comment #3
hass CreditAttribution: hass commentedComment #3.0
hass CreditAttribution: hass commentedUpdated issue summary.
Comment #4
effulgentsia CreditAttribution: effulgentsia commentedStatus change unintentional. I guess "needs review" is the appropriate status, but IMO, this should be closed as "works as designed".
Comment #5
hass CreditAttribution: hass commentedIt's a single word and these need to be ucfirst, too.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedBut it's not a single word. It's only ever shown as the @state value in the sentence
Drupal.t('"@tray" tray @state.')
.Comment #7
webchickHm. 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.
Comment #8
hass CreditAttribution: hass commentedUps. Remove all placeholders and use both strings in their full length and wording. This is a context sensitive bug.
Comment #9
hass CreditAttribution: hass commentedOr 'Tray "@tray" opened'
Comment #10
alexpottedit: x-post... removed due to irrelevancy
Comment #12
alexpottThis issue is moving too quick for me :)
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedAh, 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.
andTray %tray closed.
would be even better, no? Or are % placeholders no good within Drupal.announce()?Comment #14
markhalliwellAddressed @hass' concerns in #8 and #9
Comment #15
hass CreditAttribution: hass commentedNow, with the full sentence strings we do not need the opened/closed strings any longer isn't it?
Comment #16
markhalliwellNope, couldn't find any other code that used them. Removed.
Comment #17
effulgentsia CreditAttribution: effulgentsia commented#16 looks good to me, but I'll let hass RTBC or provide further feedback.
Comment #18
hass CreditAttribution: hass commentedThanks 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.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedI'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.
Comment #20
nod_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 :)
Comment #21
markhalliwellThanks @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.
Comment #22
webchickCool, this looks much better, thanks!
Committed and pushed to 8.x.
Comment #23
jessebeach CreditAttribution: jessebeach commentedRemoving the
sprint
tag.Comment #24
Wim LeersThis may have fixed the problem, but it introduced another one.
jessebeach carefully crafted
toolbar.js
such thatoptions.strings
could be overridden viadrupalSettings.toolbar.strings
. The changes made here broke that. So either the changes made should be moved intooptions.strings
, or we should get rid ofoptions
altogether.Comment #25
hass CreditAttribution: hass commentedWhy are you overriding a translatable string this way? There are other ways to override translatable strings... see https://drupal.org/project/stringoverrides
Comment #26
Wim LeersIf 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.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedThe 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.
Comment #28
Wim LeersA new issue is good. Even if the inconsistency already existed, it irrefutably was made worse.
Comment #29.0
(not verified) CreditAttribution: commentedUpdated issue summary.