again, this is a trivial change with no visual or performance implications. Text & patch should be reviewed & then rtbc it I expect.

CommentFileSizeAuthor
#10 trigger_select_labels_v4.patch757 bytesmgifford
#7 trigger_select_labels_v3.patch781 bytesmgifford
#2 trigger_select_labels_v2.patch781 bytesmgifford
trigger_select_labels_v1.patch782 bytesmgifford

Comments

Jeff Burnz’s picture

Jeff Burnz commented
Status: Active » Needs work 
+++ modules/trigger/trigger.admin.inc	15 Aug 2010 17:14:25 -0000
@@ -180,6 +180,8 @@ function trigger_assign_form($form, $for
+      '#title_display' => 'invisible', ¶

looks like trailing whitespace

Powered by Dreditor.

mgifford’s picture

mgifford
he/him
Primary language English
commented
StatusFileSize
new trigger_select_labels_v2.patch781 bytes

ok, here's the patch with the space removed.

Everett Zufelt’s picture

Everett Zufelt commented
Status: Needs work » Needs review

bot?

Status: Needs review » Needs work
Issue tags: -Accessibility

The last submitted patch, trigger_select_labels_v2.patch, failed testing.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs work » Needs review
Issue tags: +Accessibility

#2: trigger_select_labels_v2.patch queued for re-testing.

Everett Zufelt’s picture

Everett Zufelt commented

Looks good. Provided that users can define an action should we be using @placeholder instead of !placeholder? I'm not sure how to confirm that these types of things have been sanitized, and prefer to err on the side of caution.

* don't know how = 80% don't want to trust the other module not to have a bug at some point, 20% can't be bothered to check (honesty).

mgifford’s picture

mgifford
he/him
Primary language English
commented
StatusFileSize
new trigger_select_labels_v3.patch781 bytes

Ok, it's over to @ now. With this hopefully it can be RTBC'd when the bot clears it.

Everett Zufelt’s picture

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

Looks good. RTBC

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Needs work 
+++ modules/trigger/trigger.admin.inc	15 Aug 2010 17:14:25 -0000
@@ -180,6 +180,8 @@ function trigger_assign_form($form, $for
+      '#title' => t('List of trigger actions when @description', array('@description' => $label)),

Why "List of"...?

"Add action to invoke when @description"

would make more sense to me.

Furthermore, did you double-check that "when" is not output twice this way? I thought that $label would already start with "When..."

Powered by Dreditor.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new trigger_select_labels_v4.patch757 bytes

Another super simple label addition. Thanks @sun for the improvement in the text!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Closed (duplicate)

Please merge this patch into #882694: Add missing form element titles for accessibility. Thanks.