Skip to content
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

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

mkrecek234
Copy link
Contributor

@mkrecek234 mkrecek234 commented Aug 31, 2022

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:
image

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.

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');
Copy link
Member

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?

Copy link
Member

@mvorisek mvorisek left a 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

@mkrecek234
Copy link
Contributor Author

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

@mvorisek mvorisek marked this pull request as draft September 1, 2022 07:55
@mvorisek mvorisek changed the title Add support to add label to menugroup Add support to add label to menu group Sep 1, 2022

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');
Copy link
Member

@mvorisek mvorisek Apr 3, 2024

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'])) {
Copy link
Member

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.

Copy link
Member

@mvorisek mvorisek Apr 3, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants