? ? Pending

User tests: Successful: Unsuccessful:

avatar chmst
chmst
30 Jun 2021

Pull Request for Issue #34639 .

Summary of Changes

Enable usage of icons only in frontend menus. The user can now define an icon (for example icon-home or fas fa-user ) and build a menu using only icons without text.

NOTE: This PR does NOT care for code which seems copied for many years from one menuType to the other and results in lots of code.

The PR

  • Adds a new field to all menu types: menu_icon_css
  • Uses this in all menuTypes in the same way - this means that the menuItems which now are different in some way now have the same behaviour.

A menuItem can be just the title
or be an image with or without the title
or be an icon with or without the title (this is new).

If a menu Item is only an icon, the menuitem title is added visually hidden (a11y)
If a menuItem is not hidden, the icon gets the aria-hidden="true"

Testing Instructions

Build menus for example with blog sample data.
Change menu Items in main menu and blog menu (dropdown) and play with params:

Actual result BEFORE applying this Pull Request

Before patch- see #34639 .

Expected result AFTER applying this Pull Request

Example

grafik

Documentation Changes Required

yes

avatar chmst chmst - open - 30 Jun 2021
avatar chmst chmst - change - 30 Jun 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 30 Jun 2021
Category Administration com_menus Language & Strings Modules Front End Templates (site)
avatar dgrammatiko
dgrammatiko - comment - 30 Jun 2021

Imposing loading the font awesome icon in the front end due to some option on the back end is a terrible idea, (also this will never work on any template that doesn't load any icon fonts).
Either:

  • use Jlayouts for the markup
  • use svg icons, it's 2021 and we should stop using Icon fonts in the front end!
avatar brianteeman
brianteeman - comment - 30 Jun 2021

if you want to use an svg you can already do this.
this pr does not impose font awesome at all. it will work with any icon font the template wants to use
obviously the template would have to support an icon font for it to work

avatar brianteeman
brianteeman - comment - 30 Jun 2021

I have tested this and it does exactly what it says it will do. This is a good addition

avatar chmst
chmst - comment - 30 Jun 2021

@dgrammatiko we don't add icon fonts.

The issue was: users now add icons via the class for the link (see the issue #30793, #32471 and others ).
This causes a11y error and makes it impossible to build a menu with icons only.
This PR only let the user separate the icon from menu title.

I agree that this should be a Layout - at the moment the code is repeated in different places. But this is not in scope of this PR

5aa44b1 30 Jun 2021 avatar chmst cs
avatar chmst chmst - change - 30 Jun 2021
Labels Added: ? ?
avatar brianteeman
brianteeman - comment - 30 Jun 2021

To avoid confusion with people trying to type a path to an icon etc why not label it icon class

image

avatar chmst
chmst - comment - 30 Jun 2021

@brianteeman thank you, will do in the next step. There is another language string
COM_MENUS_ITEM_FIELD_ICON_TITLE_LABEL="Link Icon Class"
Maybe we can use this one in all layouts.

avatar ChristineWk
ChristineWk - comment - 30 Jun 2021

I have tested this and it does exactly what it says it will do. This is a good addition

confirm above.

@dgrammatiko we don't add icon fonts.

The issue was: users now add icons via the class for the link (see the issue #30793, #32471 and others ).
This causes a11y error and makes it impossible to build a menu with icons only.
This PR only let the user separate the icon from menu title.

I agree that this should be a Layout - at the moment the code is repeated in different places. But this is not in scope of this PR

tested on 4.0.0-rc4-dev and confirm:
a) icon, e.g. fa fa-home & no title = OK.
b) icon, e.g. fa fa-search & no title = OK, but changes the font, as it is class for a link. (it's known, see above issues)

Above tests, shows only a summary.

avatar chmst
chmst - comment - 30 Jun 2021

@ChristineWk thanks for testing.
Your case b) If the user enters an icon class into the link - this is a problem (the same as in blog layout, if a user says num_columns = 3 and enters columns-2 into the class field. No clue what to do then.

@brianteeman your opinion, please: We cannot avoid that a user adds an icon class AND an image. I had the image with higher proirity.

These language keys exist already
COM_MENUS_ITEM_FIELD_ICON_TITLE_DESC="If an icon class is entered, it takes precedence over the link image."
COM_MENUS_ITEM_FIELD_ICON_TITLE_LABEL="Link Icon Class"

avatar brianteeman
brianteeman - comment - 1 Jul 2021

We cannot avoid that a user adds an icon class AND an image.

Why not?

			<field
				name="menu_image"
				type="media"
				label="COM_MENUS_ITEM_FIELD_MENU_IMAGE_LABEL"
				showon="menu-anchor_icon:"
			/>

Also for consistency please rename the new fields menu_* instead of menu-*

avatar joomla-cms-bot joomla-cms-bot - change - 4 Jul 2021
Category Administration com_menus Language & Strings Modules Front End Templates (site) Administration com_menus Modules Front End Templates (site)
avatar ChristineWk
ChristineWk - comment - 4 Jul 2021

With Patch got now (for menus) in frontend:

Notice: Undefined property: Joomla\CMS\Menu\MenuItem::$menu_icon in /example.org/templates/cassiopeia/html/mod_menu/dropdown-metismenu_component.php on line 44 link of menu items

will try to load a new Nightly and try to remove the overrides I did before

avatar chmst chmst - change - 4 Jul 2021
The description was changed
avatar chmst chmst - edited - 4 Jul 2021
avatar chmst
chmst - comment - 4 Jul 2021

@brianteeman the showon is possible of course, but li feel it more confusing than helpful, especially if a user changes his mind while he is building a menu.

The more I dive into these scripts, the more I see how it is not consistent. For example - the menuItem title also is used as alt-text for the image. This is not really wrong, but also is not as clear as we have it now for other images.

As @dgrammatiko says, these different menuIten type layouts should be improved, but not now in J4.0

avatar chmst chmst - change - 7 Jul 2021
Labels Removed: ?
avatar brianteeman brianteeman - test_item - 7 Jul 2021 - Tested successfully
avatar brianteeman
brianteeman - comment - 7 Jul 2021

I have tested this item successfully on 5a8291c


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34658.

avatar ChristineWk
ChristineWk - comment - 8 Jul 2021

Still getting:

Notice: Undefined property: Joomla\CMS\Menu\MenuItem::$menu_icon in /example.org/templates/cassiopeia/html/mod_menu/dropdown-metismenu_component.php on line 44 link of menu items

Tried also with Prebuilt Package and then installed Nightly from today.

avatar chmst
chmst - comment - 8 Jul 2021

Thanks @ChristineWk!
Don't know why this was not in the latest commit. It happened when there were old menuItems.

avatar ChristineWk
ChristineWk - comment - 8 Jul 2021

Thanks @chmst
Notice has disappeared & the font in the Search menu is also back to normal.
But you can still see the title. Will keep trying.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34658.

avatar chmst
chmst - comment - 8 Jul 2021

I did not change the title. This is not in scope of this PR.

avatar chmst chmst - change - 8 Jul 2021
The description was changed
avatar chmst chmst - edited - 8 Jul 2021
avatar N6REJ
N6REJ - comment - 9 Jul 2021

glad to see this becoming part of core. I used to have to do work arounds to make this happen.

avatar Quy
Quy - comment - 14 Jul 2021

No translation for menu type URL: COM_MENUS_ITEM_FIELD_ICON_LABEL

avatar mariantanase
mariantanase - comment - 15 Jul 2021

I have tested this item successfully on #34658

avatar chmst chmst - change - 15 Jul 2021
Labels Added: ?
avatar chmst
chmst - comment - 15 Jul 2021

Changed as reqired. Thank you @Quy!

Thanks also @mariantanase for testing. I have improved the code with suggestions. It would be great if you could repeat the test (when @Quy has approved the changes)

avatar chmst chmst - change - 15 Jul 2021
The description was changed
avatar chmst chmst - edited - 15 Jul 2021
avatar chmst chmst - change - 16 Jul 2021
Labels Added: ?
Removed: ?
avatar mariantanase
mariantanase - comment - 16 Jul 2021

I have re-tested this item successfully on #34658

Anyway, I think is better to say:
"If a Link Icon Class is entered, it takes precedence over the Link Image."
instead of:
"If an icon class is entered, it takes precedence over the link image."

Or, better that that, consider to add that text as a tooltip for "Link Icon Class", to reduce the vertical space between two fields.

avatar chmst
chmst - comment - 16 Jul 2021

Thanks for testing. At the moment no changes in language strings are possible #34685 . So you could open an issue for a later version.

avatar brianteeman
brianteeman - comment - 12 Aug 2021

__Could someone remove the "updates requested" label please

avatar hans2103
hans2103 - comment - 18 Oct 2021

I have tested this item successfully on 97fee99


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34658.

avatar hans2103 hans2103 - test_item - 18 Oct 2021 - Tested successfully
avatar richard67 richard67 - alter_testresult - 18 Oct 2021 - mariantanase: Tested successfully
avatar richard67
richard67 - comment - 18 Oct 2021

@mariantanase I have altered your test result in the issue tracker so it is properly counted. Next time please us the "Test this" button in the issue tracker to submitt your test result. Making a comment with just a green check mark isn't conuted right by our tools. Thanks in advance, and thanks for testing.

avatar richard67 richard67 - change - 18 Oct 2021
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 18 Oct 2021

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34658.

avatar richard67
richard67 - comment - 18 Oct 2021

@wilsonge @bembelimen Would you say this is a bug fix so it can go into 4.0.x, or is it a new feature so it has to go into 4.1? I've set the RLDQ label because I think it needs that decision.

avatar bembelimen
bembelimen - comment - 9 Nov 2021

4.1 from my point of view.

avatar chmst
chmst - comment - 9 Nov 2021

It is a new feature from my point of view

avatar bembelimen bembelimen - change - 9 Nov 2021
Labels Added: ? ? ?
Removed: ? ?
avatar bembelimen bembelimen - change - 9 Nov 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-11-09 17:57:28
Closed_By bembelimen
Labels Added: ?
Removed: ? ?
avatar bembelimen bembelimen - close - 9 Nov 2021
avatar bembelimen bembelimen - merge - 9 Nov 2021
avatar bembelimen
bembelimen - comment - 9 Nov 2021

Thx

Add a Comment

Login with GitHub to post a comment