-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support to add label to menu group #1847
base: develop
Are you sure you want to change the base?
Conversation
src/Menu.php
Outdated
@@ -121,6 +121,8 @@ public function addGroup($name, string $template = 'menugroup.html') | |||
|
|||
if (isset($name['icon'])) { | |||
Icon::addTo($group, [$name['icon']], ['Icon'])->removeClass('item'); | |||
} elseif (isset($name['label'])) { | |||
Label::addTo($group, [$name['label']], ['Icon'])->removeClass('item')->addClass('mini'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you say this is supported somewhere and somewhere not - can we dedup the code at least with private method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need at least a test in demos - to cover the added lines
It is supported for menu items, but not for groups - groups were added outside of Fomantic UI, whereas menuitems are rendered based on standard FUI. Will work on demo |
bc372fe
to
70c8a14
Compare
|
||
if ($title !== null) { | ||
$group->template->set('title', $title); | ||
} | ||
|
||
if (isset($name['icon'])) { | ||
Icon::addTo($group, [$name['icon']], ['Icon'])->removeClass('item'); | ||
} elseif (isset($name['label'])) { | ||
Label::addTo($group, [$name['label']], ['Icon'])->removeClass('item')->addClass('tiny'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for menu item the size is standard (class is unspecified) - ie. the font size is 12 px
for menu group, the size should not be smaller (tiny is 9 px)
related with https://github.com/atk4/ui/pull/1847/files#r1548874861 which should be solved first/instead
|
||
if ($title !== null) { | ||
$group->template->set('title', $title); | ||
} | ||
|
||
if (isset($name['icon'])) { | ||
Icon::addTo($group, [$name['icon']], ['Icon'])->removeClass('item'); | ||
} elseif (isset($name['label'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caveat: It would be better if the items were right-aligned (the icon or the label) - for menu items (see below), they are right aligned, so the label spreads out to the left. If anyone has a hint, how to set classes for that please contribute.
The solution is probably to implement the "menu group item" the same way as "menu item", currently the HTML/CSS for "menu group item" is using CSS/FUI columns, but "menu item" right float positioning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in 93ac62f I have commit debug code to debug/unify this
also IMO Maestro layout should be integrated into parametrized Admin layout
27b601d
to
93ac62f
Compare
Situation:
Today, only icons are supported for menu groups. Menu items however in Admin or Maestro layout can show either an icon or a label.
PR:
Introduce to add label also for menu groups.
Sample Usage:
$layout->addMenuGroup(['Tickets', 'label' => ['1', 'class.red' => true]]);
Look:
Caveat: It would be better if the items were right-aligned (the icon or the label) - for menu items (see below), they are right aligned, so the label spreads out to the left. If anyone has a hint, how to set classes for that please contribute.