(opening this one as a reply to this comment from #995446: Update the 7.x porting guided by upgrade.boombatower.com that is now closed-fixed)

Freso:

Since the menu item is placed underneath "Language", it would be superfluous to have "Language icons" as a child of it... (IMHO)

Still, the menu item for adding a language is: Configuration > Regional & language > Languages > Add language
not simply: Configuration > Regional & language > Languages > Add

So, -even simply for consistency- instead of: Configuration > Regional & language > Languages > Icons
,we should have: Configuration > Regional & language > Languages > Language icons

Plus, please check how having it simply as 'Icons' confuses people in cases when the menu is misplaced (either intentionally- if used in a special menu containing favorite site links or because of bugs like #550254: Menu links are sometimes not properly re-parented)...

CommentFileSizeAuthor
#17 languageicons-update_menu_link_title-1003278-17-do-not-test.patch886 bytessourabh.singhal
#11 after.png46.74 KBlomasr
#9 languageicons-update_menu_link_title-1003278-9-do-not-test.patch895 bytessourabh.singhal
#5 module_configuration_pages.png55.63 KBklonos
#2 change_the_config_page_title_to_a_more_descriptive_one.png29.97 KBklonos
language_icons_or_simply_icons.png54.49 KBklonos

Comments

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented

...one OTOH might of course argue that it is simply 'List' and not 'List languages' ;)

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented
StatusFileSize
new change_the_config_page_title_to_a_more_descriptive_one.png29.97 KB

...as for the configuration page title, again images are louder than words:

Freso’s picture

Freso commented
Issue tags: +Needs usability review

I must admit you haven't really convinced me, not the least due to your own comment #1. ;)

The parent misplacement has only happened because you've used a patch that moved it somewhere it was never intended to be. Hm. Let's see if I can find a usability person to look at this.

Bojhan’s picture

Bojhan commented

I dont get it, why would we do this? Because of potetional bugs or alterations? In the latter it should be adjusted, but otherwise I do not see a reason why we would.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented
StatusFileSize
new module_configuration_pages.png55.63 KB

For the renaming of the menu item -as I've said before- it is pretty much a mater of not confusing it with other menu items. I agree that since the item is under the Languages menu, this is very unlikely to happen, but still generalization of terms is not good practice in general (term might be reserved for core for example). There are currently projects that try to add icons to the core UI and we might conflict with them in the future. So I'm merely suggesting to define the item specifically.

As for the config page title, it simply is easier to identify the page at a glance + that is how other modules do it. I have gone through the ones I currently have installed in my test setup alphabetically (only up to 'f') and here is a screenshot that shows how they all use the module's name for their config page. That's why I guess I am used to expecting this in general.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented

...you might have already noticed how I use menu_breadcrumb to have these pretty breadcrumb on to of the page. See how all the breadcrumbs look nice and make sense for all the modules in the screenie? Well, the way languageicons would display is:

Home › Administration › Configuration › Regional and language › Languages › Configuration

doesn't this make more sense instead?:

Home › Administration › Configuration › Regional and language › Languages › Language icons

yoroy’s picture

yoroy commented
Issue summary: View changes
Issue tags: -Needs usability review +Usability
sourabh.singhal’s picture

sourabh.singhal commented
Assigned: Unassigned » sourabh.singhal
sourabh.singhal’s picture

sourabh.singhal commented
Status: Active » Needs review
StatusFileSize
new languageicons-update_menu_link_title-1003278-9-do-not-test.patch895 bytes

Hi,

I have made the changes. Please review the patch.

PFA patch file.

Thanks
Sourdrup

Status: Needs review » Needs work

The last submitted patch, 9: languageicons-update_menu_link_title-1003278-9-do-not-test.patch, failed testing.

lomasr’s picture

lomasr commented
StatusFileSize
new after.png46.74 KB

Applied the patch in #9 . It worked cleanly . Please see the after.png.

lomasr’s picture

lomasr commented
Status: Needs work » Needs review
sourabh.singhal’s picture

sourabh.singhal commented

Hi lomasr,

Thanks for reviewing the patch, it seems the bug is fixed.

@klonos, can you please let me know, in case if you need any thing else from my side.
Do let me know when you will be able to close this ticket.

Thanks
Sourdrup

nikhilsukul’s picture

nikhilsukul commented
Assigned: sourabh.singhal » Unassigned
Status: Needs review » Reviewed & tested by the community

Thanks @sourdrup,

It is working fine for me as well

pfrenssen’s picture

pfrenssen
Location Sofia
commented 
+++ b/languageicons.admin.inc
@@ -13,6 +13,7 @@
+  drupal_set_title('Language icons settings/configuration');

Wouldn't this be better either 'settings' or 'configuration' but not both at the same time? They both mean the same thing.

sourabh.singhal’s picture

sourabh.singhal commented
Assigned: Unassigned » sourabh.singhal
sourabh.singhal’s picture

sourabh.singhal commented
StatusFileSize
new languageicons-update_menu_link_title-1003278-17-do-not-test.patch886 bytes

Hi pfrenssen,

I really appreciate for your feedback, I have updated my patch and set title to : drupal_set_title('Language icons configuration');

Thanks
Sourdrup

  • pfrenssen committed 8ee7c29 on 7.x-1.x authored by sourdrup 
    Issue #1003278 by sourdrup, klonos, lomasr: Clarify links to...
pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Reviewed & tested by the community » Fixed

Thanks, looks great! Committed to 7.x-1.x.

Status: Fixed » Closed (fixed)

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