-
Notifications
You must be signed in to change notification settings - Fork 8
Favs extended #206
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
Favs extended #206
Conversation
Neue Features:
- Jeder User kann individuell Buttons ausblenden (Opt-Out)
- Alle Buttons standardmäßig aktiv
- Funktioniert auch mit extern registrierten Buttons
- Button-Auswahl in den Benutzereinstellungen
Technische Änderungen:
- ButtonRegistry erweitert mit IDs und Labels
- registerButton() mit optionalen ID/Label Parametern
- getButtonsOutput() filtert nach User-Präferenzen
- getAvailableButtons() für Config-Seite
- Config-Seite: Button-Management Section hinzugefügt
- Lang-Keys für DE und EN ergänzt
Verwendung für externe Buttons:
ButtonRegistry::registerButton(
$instance,
$priority,
'my_button_id',
'Mein Button Label'
);
Problem behoben: - getPerm() existiert nicht in rex_be_page - Korrekte Methode ist getRequiredPermissions() - Anpassung in FavoriteButton::getAvailablePages() - Anpassung in FavoriteButton::parsePageKey() Alle Berechtigungsprüfungen funktionieren jetzt korrekt.
Reihenfolge geändert: 1. Struktur-Favoriten (Artikel/Kategorien) 2. AddOn-Seiten Damit stehen die wichtigeren Struktur-Favoriten mit ihren Quick-Actions immer am Anfang der Liste.
CSS-Anpassungen für rex-theme-dark: - Section-Header: Dunklerer Hintergrund (#2a2d33) - Text: Helleres Grau (#aaa) - Borders: Dunkles Grau (#404040) - Hover: Passende dunkle Farbe (#35383e) - Divider und Menu-Header angepasst Die Favoriten fügen sich jetzt harmonisch ins Dark Theme ein.
Änderungen: - Section-Divider ausgeblendet (display: none) - Header haben bereits border-top - Verhindert doppelte Linien zwischen Sections - Sauberere Darstellung in Light und Dark Theme
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.
Pull request overview
This pull request extends the favorites functionality to support AddOn backend pages in addition to structure categories, and introduces a button management system that allows users to selectively hide buttons via an opt-out configuration.
- Extended favorites to include backend addon pages alongside structure categories with sectioned display
- Added button management settings allowing users to hide individual buttons (opt-out approach)
- Updated button registration to include unique IDs and labels for configuration UI
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| pages/config.php | Adds configuration UI for addon page favorites selection and button visibility management |
| lib/Button/FavoriteButton.php | Extends favorites to support addon pages with permission checks and sectioned display |
| lib/Button/ButtonRegistry.php | Adds button ID/label metadata and filtering logic for disabled buttons |
| lang/en_gb.lang | Adds English translations for new addon pages features and button management |
| lang/de_de.lang | Adds German translations for new addon pages features and button management (contains duplicate key) |
| fragments/QuickNavigation/List.php | Updates template to conditionally render section headers and dividers |
| boot.php | Updates all button registrations to include explicit IDs and labels |
| assets/quick-navigation.css | Adds styling for section headers, dividers, and addon favorite items with dark theme support |
| .php-cs-fixer.cache | PHP-CS-Fixer cache file update (resolved merge conflict) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $listItem = ' | ||
| <div class="quick-navigation-item-row quick-navigation-addon-fav"> | ||
| <a' . rex_string::buildAttributes($attributes) . '> | ||
| <i class="' . rex_escape($pageInfo['icon']) . '" aria-hidden="true"></i> |
Copilot
AI
Dec 5, 2025
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.
[nitpick] The rex_escape() function is properly used to escape output for the addon page key and title, which prevents XSS vulnerabilities. However, the icon value at line 121 also uses rex_escape(), but icons in this context are likely CSS class names that should already be safe. While escaping doesn't hurt, ensure that the getIcon() method returns validated CSS class names and not arbitrary user input.
| quick_navigation_media_live_search_placeholder = Live-Suche... | ||
|
|
||
| # Live Search | ||
| quick_navigation_media_live_search_placeholder = Live-Suche... |
Copilot
AI
Dec 5, 2025
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.
Duplicate key definition. The key quick_navigation_media_live_search_placeholder is defined twice (lines 37 and 40), which will cause the second definition to override the first. Remove the duplicate entry.
| quick_navigation_media_live_search_placeholder = Live-Suche... |
| } | ||
| $listItems[] = '<div class="quick-navigation-section-header">' . rex_i18n::msg('quick_navigation_addon_pages') . '</div>'; | ||
|
|
||
| foreach ($addonPages as $pageKey) { |
Copilot
AI
Dec 5, 2025
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.
While is_array() check is performed, there's no validation of the array contents. If $addonPages contains invalid values (non-string elements), the parsePageKey() call at line 96 will fail with a type error in PHP 8+. Consider adding type validation or using array_filter to ensure all elements are strings before processing.
| foreach ($addonPages as $pageKey) { | |
| // Ensure only string elements are processed | |
| foreach (array_filter($addonPages, 'is_string') as $pageKey) { |
| } | ||
| } | ||
|
|
||
| // Keine Favoriten |
Copilot
AI
Dec 5, 2025
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.
[nitpick] Comment is in German while most of the codebase appears to use English comments. For consistency and broader maintainability, consider translating to English: "// No favorites"
| // Keine Favoriten | |
| // No favorites |
| $selectHtml = '<select name="config[quick_navigation_addon_favs' . $user . '][]" id="quick-navigation-config-addon-favs" class="selectpicker show-menu-arrow form-control" multiple="multiple" data-live-search="true" data-size="15" data-actions-box="false" size="10">'; | ||
|
|
||
| foreach ($availablePages as $page) { | ||
| $selected = in_array($page['key'], $selectedPages, true) ? ' selected="selected"' : ''; | ||
| $selectHtml .= sprintf( | ||
| '<option value="%s"%s>%s</option>', | ||
| rex_escape($page['key']), | ||
| $selected, | ||
| rex_escape($page['title']) | ||
| ); | ||
| } | ||
|
|
||
| $selectHtml .= '</select>'; | ||
| $n['field'] = $selectHtml; |
Copilot
AI
Dec 5, 2025
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.
[nitpick] The HTML string for the select element is built using concatenation. Consider using a more maintainable approach, such as building an array of option attributes and using a helper function or template, similar to how the category select is handled with rex_category_select at lines 51-61.
| $selectHtml = '<select name="config[quick_navigation_addon_favs' . $user . '][]" id="quick-navigation-config-addon-favs" class="selectpicker show-menu-arrow form-control" multiple="multiple" data-live-search="true" data-size="15" data-actions-box="false" size="10">'; | |
| foreach ($availablePages as $page) { | |
| $selected = in_array($page['key'], $selectedPages, true) ? ' selected="selected"' : ''; | |
| $selectHtml .= sprintf( | |
| '<option value="%s"%s>%s</option>', | |
| rex_escape($page['key']), | |
| $selected, | |
| rex_escape($page['title']) | |
| ); | |
| } | |
| $selectHtml .= '</select>'; | |
| $n['field'] = $selectHtml; | |
| // Build options array for select | |
| $options = []; | |
| foreach ($availablePages as $page) { | |
| $options[] = [ | |
| 'value' => rex_escape($page['key']), | |
| 'label' => rex_escape($page['title']), | |
| 'selected' => in_array($page['key'], $selectedPages, true), | |
| ]; | |
| } | |
| // Use fragment to render select | |
| $selectFragment = new rex_fragment(); | |
| $selectFragment->setVar('options', $options, false); | |
| $selectFragment->setVar('name', 'config[quick_navigation_addon_favs' . $user . '][]', false); | |
| $selectFragment->setVar('id', 'quick-navigation-config-addon-favs', false); | |
| $selectFragment->setVar('class', 'selectpicker show-menu-arrow form-control', false); | |
| $selectFragment->setVar('multiple', true, false); | |
| $selectFragment->setVar('attributes', [ | |
| 'data-live-search' => 'true', | |
| 'data-size' => '15', | |
| 'data-actions-box' => 'false', | |
| 'size' => '10', | |
| ], false); | |
| $n['field'] = $selectFragment->parse('core/form/select.php'); |
| <?php if (str_contains($listItem, 'quick-navigation-section-header') || str_contains($listItem, 'quick-navigation-section-divider')): ?> | ||
| <?= $listItem ?> | ||
| <?php else: ?> | ||
| <li> | ||
| <div class="quick-navigation-item"> | ||
| <?= $listItem ?> |
Copilot
AI
Dec 5, 2025
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.
The condition str_contains($listItem, 'quick-navigation-section-header') is used to detect section headers, but this is fragile. If the section header text happens to appear in a regular list item (e.g., in a category name or addon page title), it will be incorrectly treated as a header. Consider using a more robust method, such as checking if the string starts with a specific pattern or using an array structure with type indicators instead of HTML string detection.
| <?php if (str_contains($listItem, 'quick-navigation-section-header') || str_contains($listItem, 'quick-navigation-section-divider')): ?> | |
| <?= $listItem ?> | |
| <?php else: ?> | |
| <li> | |
| <div class="quick-navigation-item"> | |
| <?= $listItem ?> | |
| <?php if ($listItem['type'] === 'header'): ?> | |
| <?= $listItem['content'] ?> | |
| <?php elseif ($listItem['type'] === 'divider'): ?> | |
| <?= $listItem['content'] ?> | |
| <?php else: ?> | |
| <li> | |
| <div class="quick-navigation-item"> | |
| <?= $listItem['content'] ?> |
| if (!empty($requiredPerms) && !$user->hasPerm($requiredPerms)) { | ||
| continue; |
Copilot
AI
Dec 5, 2025
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.
The permission check uses $user->hasPerm($requiredPerms) where $requiredPerms is an array. However, according to typical REDAXO patterns, hasPerm() may expect individual permission strings rather than an array. Verify that hasPerm() correctly handles array inputs, or use $user->hasComplex() or iterate through permissions if needed.
| if (!empty($requiredPerms) && !$user->hasPerm($requiredPerms)) { | |
| continue; | |
| if (!empty($requiredPerms)) { | |
| $hasAllPerms = true; | |
| foreach ($requiredPerms as $perm) { | |
| if (!$user->hasPerm($perm)) { | |
| $hasAllPerms = false; | |
| break; | |
| } | |
| } | |
| if (!$hasAllPerms) { | |
| continue; | |
| } |
| .quick-navigation-section-divider { | ||
| display: none; | ||
| } |
Copilot
AI
Dec 5, 2025
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.
[nitpick] The .quick-navigation-section-divider class is defined but set to display: none;. This suggests it's not currently being used. Consider removing this unused CSS rule or documenting why it's kept for future use.
| .quick-navigation-section-divider { | |
| display: none; | |
| } |
| $buttonCheckboxes .= sprintf( | ||
| '<div class="checkbox"> | ||
| <label> | ||
| <input type="checkbox" name="config[quick_navigation_disabled_buttons%s][]" value="%s"%s> | ||
| %s | ||
| </label> | ||
| </div>', | ||
| $user, | ||
| rex_escape($button['id']), | ||
| $isDisabled ? ' checked="checked"' : '', | ||
| rex_escape($button['label']) | ||
| ); |
Copilot
AI
Dec 5, 2025
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.
[nitpick] The HTML string concatenation in the sprintf is mixing single and double quotes inconsistently, and the indentation in the heredoc/multiline string doesn't match the surrounding code indentation. While functional, this reduces code readability. Consider using consistent quote style and proper indentation.
| $fragment->setVar('elements', $formElements, false); | ||
| $content .= $fragment->parse('core/form/container.php'); | ||
|
|
||
| // AddOn-Seiten Favoriten |
Copilot
AI
Dec 5, 2025
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.
[nitpick] Comment is in German while most of the code uses English comments and variable names. For consistency and broader maintainability, consider translating to English: "// AddOn Pages Favorites"
| // AddOn-Seiten Favoriten | |
| // AddOn Pages Favorites |
und neue Settings um Buttons auszublenden.