Problem/Motivation

The documentation for the Drupal.theme.dropbuttonToggle function doesn't make sense. It says:

     * @param {string} [options.title]
     *   The HTML anchor title attribute and text for the inner span element.

But this theme function doesn't output a HTML anchor - it outputs a HTML button. And there isn't a title attribute in the output either.

Proposed resolution

Update the docblock, so it matches the actual output:

  • options.title is the label for a HTML button (not for a HTML anchor).
  • and remove the part about the title attribute, because there isn't a title attribute in the output.

Remaining tasks

  • Patch the docblock in core/misc/dropbutton.es6.js
  • Note: the changes in the ES6 file are comments only, which are stripped out when transpiled. So we're not expecting a change in the ES5 JS file. Recompile core/misc/dropbutton.js (see javascript workflow instructions).

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#17 docblock-for-dropbutton-2972445-17.patch524 bytessudheeshps
#11 docblock-for-dropbutton-2972445-9.patch530 bytessudheeshps
#8 docblock-for-dropbutton-2972445-8.patch558 bytessudheeshps
#4 2972445-4.patch557 bytesAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andrewmacpherson created an issue. See original summary.

andrewmacpherson’s picture

andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commented
Issue summary: View changes
Issue tags: +Nwdug_may18

This is an easy documentation issue for the Manchester sprint. Also a good opportunity to learn about how we manage the ES6 and compiled JS files in drupal core.

andrewmacpherson’s picture

andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commented
Issue tags: +Novice
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous as a volunteer commented
FileSize
2972445-4.patch557 bytes

Hi @andrewmacpherson,

I've added a patch for the docblock change in the es6 file. Unfortunately, when I run yarn run build:js there doesn't seem to be any changes made to dropbutton.js. Am I missing anything?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous as a volunteer commented
Status: Active » Needs review
GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

@sjhuda Comments are removed in the transpile process.

andrewmacpherson’s picture

andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commented
Title: Dockblock for dropbutton theme function is incorrect. » Docblock for dropbutton theme function is incorrect.
Issue summary: View changes
Status: Needs review » Needs work

@sjhuda -
I've added a patch for the docblock change in the es6 file. Unfortunately, when I run yarn run build:js there doesn't seem to be any changes made to dropbutton.js. Am I missing anything?

Ah, the changes are solely in the documentation comments of the ES6 file, and the comments are stripped out when transpiling the JS file. So this is fine - we're not expecting a change in the compiled JS file.

-     *   The HTML anchor title attribute and text for the inner span element.
+     *   The HTML button label for the inner span element.

I think we can go a bit further and just say "The HTML button label." There's nothing special about the internal span structure, as far as this comment goes. Can you roll another patch?

sudheeshps’s picture

sudheeshps CreditAttribution: sudheeshps at TATA Consultancy Services commented
FileSize
docblock-for-dropbutton-2972445-8.patch558 bytes

@andrewmacpherson,

Changed comment as mentioned. Please check and let me know

sudheeshps’s picture

sudheeshps CreditAttribution: sudheeshps at TATA Consultancy Services commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: docblock-for-dropbutton-2972445-8.patch, failed testing. View results

sudheeshps’s picture

sudheeshps CreditAttribution: sudheeshps at TATA Consultancy Services for Johnson & Johnson commented
FileSize
docblock-for-dropbutton-2972445-9.patch530 bytes

Updated patch

sudheeshps’s picture

sudheeshps CreditAttribution: sudheeshps at TATA Consultancy Services commented
Status: Needs work » Needs review
mahalingam_cs’s picture

mahalingam_cs CreditAttribution: mahalingam_cs at TATA Consultancy Services commented
Assigned: Unassigned » mahalingam_cs
mahalingam_cs’s picture

mahalingam_cs CreditAttribution: mahalingam_cs at TATA Consultancy Services for Johnson & Johnson commented
Status: Needs review » Reviewed & tested by the community

Applied patch from #11 and was successful.

"The HTML anchor....." is updated to "The HTML button label."

andrewmacpherson’s picture

andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commented

Thanks, patch #11 looks great.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/misc/dropbutton/dropbutton.es6.js
@@ -214,7 +214,7 @@
+     *   The HTML button label.

I think rather than using label which has quite a few meanings in HTML let's go for The button text. - I think HTML is redundant here - this is a theme function after all.

The wording button text comes from https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button

However, it is worth noting that leaving the button text visually apparent can aid people who may not be familiar with the icon's meaning understand the button's purpose. This is especially relevant for people who are not as technologically sophisticated, or who may have different cultural interpretations for the imagery the icon button uses.

sudheeshps’s picture

sudheeshps CreditAttribution: sudheeshps at TATA Consultancy Services for Johnson & Johnson commented
Assigned: mahalingam_cs » sudheeshps
Status: Needs work » Needs review
FileSize
docblock-for-dropbutton-2972445-17.patch524 bytes

Thanks @alexpott, I have updated the text again.

andrewmacpherson’s picture

andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Perfect, patch #17 address #16 exactly.

mahalingam_cs’s picture

mahalingam_cs CreditAttribution: mahalingam_cs at TATA Consultancy Services for Johnson & Johnson commented

+1 RTBC. Patch #17 applied cleanly and fixed the comments 16

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

Credited @andrewmacpherson and myself for comments that affected the patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 346860aff3 to 8.6.x and 5f531759ef to 8.5.x. Thanks!

  • alexpott committed 346860a on 8.6.x 
    Issue #2972445 by sudheeshps, sjhuda, andrewmacpherson, alexpott:...

  • alexpott committed 5f53175 on 8.5.x 
    Issue #2972445 by sudheeshps, sjhuda, andrewmacpherson, alexpott:...

Status: Fixed » Closed (fixed)

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