Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seppelM created an issue. See original summary.

seppelM’s picture

Status: Active » Needs review

Status: Needs review » Needs work

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

Anonymous’s picture

Seems like a duplicate of 2615476, or vice versa.

neha.gangwar’s picture

neha.gangwar’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: correct_admin_toolbar_info-2614962-5.patch, failed testing.

andrewmacpherson’s picture

The maintainer (eme) closed the other issue as a duplicate of this one.

The patch in #5 has a few problems:

  • It contains the packaging info generated by the drupal.org packaging scripts. Looks like the patch was rolled against a module release. It should be re-rolled against the dev release. See the "Packaging" heading on the version control page.
  • The description starts with the name of the module. This isn't really necessary.
  • "improve" should be "improves", and "transform" should be "transforms".

The sub-module's info file also has spelling mistakes.

chegor’s picture

Status: Needs work » Needs review
FileSize
952 bytes

Improved patch

andrewmacpherson’s picture

Status: Needs review » Reviewed & tested by the community

Looking better chegor <8•)

The drupal.org packaging info has been cleared out.

Updates descriptions of main module and submodule info files.

Improves, transforms, fonctionalities typos fixed.

Removes unrcessary repetition of the module name.

jmev’s picture

I just installed it on a local D8 site and the description reads "Admin Toolbar Extra Tools adds more fonctionalities in the admin menu.", so perhaps a global find/replace of "fonctionalities" would help resolve all occurrences.

klonos’s picture

The last patch changes the text to "Improves the default administration menu and transforms it into a drop down menus." Either the "a" is superfluous or it should be "a drop down menu" (no plural for the word "menu"). Also, the norm is "drop-down" (with a hyphen). The rest of the variations are simply "acceptable" alternatives of "drop-down".

dbt102’s picture

I use this module so much, this issue was kind of 'bugging' :-) me.

I applied the ...62-9.patch to a "git clone --branch 8.x-1.x https://git.drupal.org/project/admin_toolbar.git" pulled today, then grabbed a screen shot so you can see ...

what it looked like to start (see attached file "Admin_Toolbar_62-9.patch Screen Shot 2016-04-21 at 2.32.30 PM"

I made a couple changes to make descriptions a little clearer, and especially address @klonos #12 comment above.

see ..

What it looks like per my updates (see attached file "Admin_Toolbar_NOW_Screen Shot 2016-04-21 at 2.37.21 PM"

Rolled new patch of what it looks like NOW for testing.

dbt102’s picture

ooops ... my _62-13.patch is NOT correct.

After I posted it, then looked at the screenshots of before and after it seemed to me it could still be better. I went looking to see if I could find what the 'standard' actually is. Just by looking at the short descriptions provided by any number of modules on a particular site, its easy to see that some fall into a kind of 'rythm'.

Looks like the best guidance is here --> https://www.drupal.org/node/632280

The description starts with a verb and should be short and concise.

So I would propose this wording...

"Adds drop-down menus to the core Drupal Toolbar."

I'll roll that patch in a few minutes.

dbt102’s picture

ok ...

changed the file admin_toolbar.info.yml to "Provides a drop-down menu interface to the core Drupal Toolbar."

changed the file admin_toolbar_tools.info.yml to "Adds additional functionality to the admin menu."

see attached screenshot file --> Revised Screen Shot 2016-04-23 at 12.30.59 AM.png

I think I'm satisfied with that :-)

dbt102’s picture

Status: Reviewed & tested by the community » Needs review
andrewmacpherson’s picture

Status: Needs review » Reviewed & tested by the community
chegor’s picture

Status: Reviewed & tested by the community » Needs review

I don't think that variant from #15 is better than in #9

dbt102’s picture

Thanks for your comment #19 @chegor . Please let me explain why I proposed the variant of #16 vs #9

for name: Admin Toolbar

#9 patch --> "Improves the default administration menu and transforms it into a drop down menus."

1. The grammar of the last several words "... it into a drop down menus" is not correct.

If that phrasing is used it should be either "... it into a drop down menu" or "... it into drop down menus".

2. Admin Toolbar does not really "improve" the Drupal core Toolbar module so much as it improves the user experience when interacting with Toolbar.

3. The Admin Toolbar does kind of "transform" the 'admin' user experience.

4. The reference to "the default administration menu" should be to that of a proper noun, which in this case is the "Drupal" core "Toolbar", or the core "Drupal Toolbar", or the core "Toolbar" or I think just "Toolbar" would be correct. My preference is for "Drupal Toolbar" because if I search for that on Google its the #1 item. Whereas if I search for just "Toolbar" on google, the number 1 result is for the Google Toolbar.

#16 patch --> "Provides a drop-down menu interface to the core Drupal Toolbar."

  • romainj committed e5b7ef6 on 8.x-1.x authored by dbt102
    Issue #2614962 by dbt102, seppelM, neha.gangwar, chegor: Correct text in...
romainj’s picture

Patch in #16 has been committed.

romainj’s picture

Status: Needs review » Fixed

The last submitted patch, 13: correct_admin_toolbar_info-2614962-13.patch, failed testing.

The last submitted patch, 13: correct_admin_toolbar_info-2614962-13.patch, failed testing.

The last submitted patch, 15: correct_admin_toolbar_info-2614962-15.patch, failed testing.

Status: Fixed » Needs work

The last submitted patch, 16: correct_admin_toolbar_info-2614962-16.patch, failed testing.

The last submitted patch, 16: correct_admin_toolbar_info-2614962-16.patch, failed testing.

dbt102’s picture

Status: Needs work » Closed (fixed)

Closing out this issue since the patch has been committed