User tests: Successful: Unsuccessful:
Pull Request for Issue #34639 .
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
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"
Build menus for example with blog sample data.
Change menu Items in main menu and blog menu (dropdown) and play with params:
Before patch- see #34639 .
Example
yes
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_menus Language & Strings Modules Front End Templates (site) |
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
I have tested this and it does exactly what it says it will do. This is a good addition
@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
Labels |
Added:
?
?
|
@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.
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.
@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"
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-*
Category | Administration com_menus Language & Strings Modules Front End Templates (site) | ⇒ | Administration com_menus Modules Front End Templates (site) |
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
@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
Labels |
Removed:
?
|
I have tested this item
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.
Thanks @ChristineWk!
Don't know why this was not in the latest commit. It happened when there were old menuItems.
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.
I did not change the title. This is not in scope of this PR.
glad to see this becoming part of core. I used to have to do work arounds to make this happen.
No translation for menu type URL: COM_MENUS_ITEM_FIELD_ICON_LABEL
Labels |
Added:
?
|
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)
Labels |
Added:
?
Removed: ? |
I have re-tested this item
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.
__Could someone remove the "updates requested" label please
I have tested this item
@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.
Status | Pending | ⇒ | Ready to Commit |
RTC
@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.
4.1 from my point of view.
It is a new feature from my point of view
Labels |
Added:
?
?
?
Removed: ? ? |
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: ? ? |
Thx
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: