Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes enhance the administration menu functionality by adding a new private method in the main menu builder that constructs an "Administration" submenu with designated child items and extra metadata. The menu template is updated to incorporate extra Bootstrap classes and conditional logic to render a Sylius Plus logo or an external link icon based on data. Additionally, a new CSS class is introduced to style a badge element, setting its width to 50px. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as UI Request
participant MMB as MainMenuBuilder
participant Menu as MenuStructure
Client->>MMB: createMenu()
MMB->>Menu: Build base menu
MMB->>MMB: addAdministrationSubMenu(Menu)
MMB->>Menu: Append Administration submenu<br/>with child items and metadata
Menu-->>MMB: Menu updated
MMB-->>Client: Return complete menu
sequenceDiagram
participant Template as Twig Template
participant Logic as Conditional Checker
Template->>Logic: Check if plus_logo exists
alt plus_logo exists
Logic-->>Template: Render Sylius Plus logo image
else plus_logo absent
Template->>Logic: Check if target == "_blank"
alt target is _blank
Logic-->>Template: Render external link icon
else
Logic-->>Template: Render plain label
end
end
Template-->>User: Rendered dropdown item
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
9b6c6ad to
d875964
Compare
❌ Preview Environment deleted from BunnyshellAvailable commands:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Sylius/Bundle/AdminBundle/Menu/MainMenuBuilder.php (1)
397-417: Improve consistency in menu item naming conventionThe implementation of the Administration submenu has some inconsistencies with the established naming patterns:
- The submenu ID
addAdministration_supportuses mixed conventions (camelCase with underscore) and includes an "add" prefix unlike other menu items- The child item name
Rolesis capitalized, while other child items use lowercaseFor better consistency with the rest of the codebase, consider these changes:
private function addAdministrationSubMenu(ItemInterface $menu): void { $configuration = $menu - ->addChild('addAdministration_support') + ->addChild('administration_submenu') ->setLabel('sylius.ui.administration') ->setLabelAttribute('icon', 'tabler:lock') ; $sylius_plus = $configuration - ->addChild('Roles') + ->addChild('roles') ->setUri('https://sylius.com/plus/') ->setLinkAttribute('target', '_blank') ->setLabel('sylius.ui.roles') ->setLabelAttribute('icon', 'tabler:plus') ; $sylius_plus ->setExtra('plus_logo', '/sylius-plus-sidebar.svg') ->setExtra('safe_label', true) ; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/Sylius/Bundle/AdminBundle/Resources/assets/images/sylius-plus-sidebar.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
src/Sylius/Bundle/AdminBundle/Menu/MainMenuBuilder.php(2 hunks)src/Sylius/Bundle/AdminBundle/Resources/assets/entrypoint.js(1 hunks)src/Sylius/Bundle/AdminBundle/Resources/assets/styles/_navbar.scss(1 hunks)src/Sylius/Bundle/AdminBundle/templates/shared/crud/common/sidebar/menu/menu.html.twig(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.3, Symfony ^7.1 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.3, Symfony ^7.1 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.3, Symfony ^7.1 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (PostgreSQL) / Non-JS, PHP 8.3, Symfony ^7.1, PostgreSQL 16.4
- GitHub Check: End-to-end tests (PostgreSQL) / Non-JS, PHP 8.2, Symfony ^6.4, PostgreSQL 15.8
- GitHub Check: Frontend / NodeJS 20.x
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.3, Symfony ^7.1, MariaDB 11.4.3, State Machine Adapter winzou_state_machine
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.3, Symfony ^7.1, MariaDB 11.4.3, State Machine Adapter symfony_workflow
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.2, Symfony ^6.4, MariaDB 10.11.9, State Machine Adapter symfony_workflow
- GitHub Check: Packages / PHP 8.3, Symfony ^7.1
- GitHub Check: Packages / PHP 8.2, Symfony ^6.4
🔇 Additional comments (4)
src/Sylius/Bundle/AdminBundle/Resources/assets/entrypoint.js (1)
29-29: Looks good: Added import for Sylius Plus logoThe addition of the Sylius Plus sidebar SVG import is appropriate for the objectives of this PR, which aims to enhance the admin UI by displaying the Sylius Plus logo in the navigation.
src/Sylius/Bundle/AdminBundle/Resources/assets/styles/_navbar.scss (1)
29-31: Looks good: New CSS class for the Plus badgeThe new CSS class
.navbar-plus-badgewith a fixed width of 50px is appropriate for consistently sizing the Sylius Plus logo in the navigation menu.src/Sylius/Bundle/AdminBundle/Menu/MainMenuBuilder.php (1)
44-44: Looks good: Integration of the new submenuThe invocation of the new method in the menu creation chain is appropriate and follows the existing pattern.
src/Sylius/Bundle/AdminBundle/templates/shared/crud/common/sidebar/menu/menu.html.twig (1)
92-98:Details
✅ Verification successful
Looks good: Enhanced dropdown item with conditional logo rendering
The changes to the template are well-implemented:
- The added Bootstrap utility classes improve the layout
- The conditional rendering of the Plus logo is implemented correctly
- Backward compatibility is maintained with the existing external link icon display
🌐 Web query:
What are Bootstrap's justify-content-lg-between and gap-2 utility classes used for?💡 Result:
Bootstrap's
justify-content-lg-betweenandgap-2are utility classes for controlling layout in responsive designs:justify-content-lg-between
This flexbox utility:
- Applies
justify-content: space-betweenonly on large screens (≥ 992px) [4][10]- Distributes flex items with:
- First item at container start
- Last item at container end
- Equal spacing between items [1][10]
- Becomes active at breakpoint
lgwhile using default alignment on smaller screens [3][7]Example usage:
<div class="d-flex justify-content-lg-between"> <div>Item 1</div> <div>Item 2</div> <div>Item 3</div> </div>gap-2
This spacing utility:
- Adds
0.5remgap between grid/flex items
- (Based on Bootstrap's spacer value where
$spacer = 1rem) [2][5][6]- Applies to both horizontal and vertical spacing
- Works with both CSS Grid (
display: grid) and Flexbox layouts [6]<div class="d-flex gap-2"> <div>Item A</div> <div>Item B</div> </div>Combined Use Case
When used together:
<div class="d-flex justify-content-lg-between gap-2"> <div>Left item</div> <div>Middle item</div> <div>Right item</div> </div>This creates:
- Large screens: Items spread edge-to-edge with equal spacing between them
- Smaller screens: Default alignment (usually left) with consistent 0.5rem gaps
- Responsive spacing maintained across all screen sizes [6][9]
Key benefits include eliminating manual margin calculations and ensuring consistent spacing while maintaining responsive layout control [2][5]. The gap utility is particularly useful in modern grid/flex layouts where direct child margins can cause unintended overflow issues [6].
Citations:
- 1: https://getbootstrap.com/docs/4.6/utilities/flex/
- 2: https://getbootstrap.com/docs/5.0/utilities/spacing/
- 3: https://bootstrapshuffle.com/classes/flexbox/justify-content-*-between
- 4: https://getbootstrap.com/docs/5.0/utilities/flex/
- 5: https://mdbootstrap.com/docs/standard/utilities/spacing/
- 6: https://getbootstrap.com/docs/5.3/utilities/spacing/
- 7: https://www.w3schools.com/bootstrap4/tryit.asp?filename=trybs_flex-justify-between-responsive
- 8: https://getbootstrap.com/docs/5.0/utilities/api/
- 9: https://www.vincentschmalbach.com/bootstrap-5-utility-classes/
- 10: https://fastbootstrap.com/docs/justify-content/
Approval: Enhanced Dropdown Responsiveness and Logo Rendering Verified
The updated template looks solid:
- The use of Bootstrap's
justify-content-lg-betweencorrectly applies space-between layout on large screens, ensuring items are spread out appropriately.- The
gap-2class reliably provides a consistent 0.5rem gap between elements.- The conditional rendering of the Plus logo (or external link icon for
_blanktargets) is implemented correctly, maintaining backward compatibility.
d875964 to
84bc1cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Sylius/Bundle/AdminBundle/Menu/MainMenuBuilder.php(2 hunks)src/Sylius/Bundle/AdminBundle/templates/shared/crud/common/sidebar/menu/menu.html.twig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Sylius/Bundle/AdminBundle/templates/shared/crud/common/sidebar/menu/menu.html.twig
🔇 Additional comments (3)
src/Sylius/Bundle/AdminBundle/Menu/MainMenuBuilder.php (3)
44-44: New menu call integration looks goodThe integration of the new Administration submenu into the main menu structure is consistent with how other submenus are added.
397-417: Verify the purpose of this submenu and potential duplicationThis administration submenu links to the same URL as the "sylius_plus" item in the "Official Support" submenu. Both point to "https://sylius.com/plus/", which may cause confusion for users.
Consider either:
- Adding different menu items to the administration submenu that are actually related to administration
- Removing this new submenu if it's intended to simply provide another way to access Sylius Plus
Please check if this is intentional duplication or if the new submenu should contain different administrative functionality.
413-416:Details
✅ Verification successful
Verify SVG path and review usage
The code sets
plus_logoto/sylius-plus-sidebar.svg, but this needs to be confirmed:
- Does this SVG file exist at the specified path?
- Is the template designed to handle the
plus_logoextra parameter?
🏁 Script executed:
#!/bin/bash # Check if the SVG file exists fd -t f "sylius-plus-sidebar.svg" # Check if any template is using the "plus_logo" extra parameter rg "plus_logo" --type=twig -A 5 -B 5Length of output: 2176
SVG resource verified and template usage confirmed.
- The SVG file exists at
src/Sylius/Bundle/AdminBundle/Resources/assets/images/sylius-plus-sidebar.svg.- The Twig template (
src/Sylius/Bundle/AdminBundle/templates/shared/crud/common/sidebar/menu/menu.html.twig) correctly checks forplus_logoand renders it when available.
c6c41e5 to
84bc1cd
Compare
01b1543 to
a3abc51
Compare
2393e7b to
d86ab9d
Compare
d86ab9d to
82624da
Compare
src/Sylius/Bundle/AdminBundle/Resources/config/app/twig_hooks/admin_user/create.yaml
Outdated
Show resolved
Hide resolved
Updated plus utm link Fixes #17827
Added support for displaying Sylius Plus logo in the admin navbar via menu.
Included an example usage in the Administration section of the main menu.
Summary by CodeRabbit
New Features
Style