Conversation
WalkthroughThis pull request introduces a fullscreen feature for the Sylius Admin Bundle. The implementation involves adding a new JavaScript script to handle fullscreen toggling, updating the translation file with a new key for "Full screen", and modifying the order items template to include a fullscreen button and container attribute. The changes provide users with the ability to expand specific sections of the admin interface to full screen mode. Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Bunnyshell Preview Environment deletedAvailable commands:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Sylius/Bundle/AdminBundle/templates/order/show/content/sections/items.html.twig (1)
1-1: Consider adding ARIA attributes for accessibility.The fullscreen container should have appropriate ARIA attributes to improve accessibility.
-<div class="card mb-3" data-fullscreen-container="orders"> +<div class="card mb-3" data-fullscreen-container="orders" role="region" aria-label="{{ 'sylius.ui.items'|trans }}">src/Sylius/Bundle/AdminBundle/Resources/assets/scripts/fullscreen.js (1)
1-25: Consider adding fullscreen change event listener.Add an event listener for fullscreen changes to update UI state (e.g., button icon).
+document.addEventListener('fullscreenchange', () => { + document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => { + const icon = btn.querySelector('svg'); + if (document.fullscreenElement) { + icon.innerHTML = '/* minimize icon */'; + } else { + icon.innerHTML = '/* maximize icon */'; + } + }); +}); document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Sylius/Bundle/AdminBundle/Resources/assets/entrypoint.js(1 hunks)src/Sylius/Bundle/AdminBundle/Resources/assets/scripts/fullscreen.js(1 hunks)src/Sylius/Bundle/AdminBundle/Resources/translations/messages.en.yml(1 hunks)src/Sylius/Bundle/AdminBundle/templates/order/show/content/sections/items.html.twig(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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) / 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 Chromedriver, 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) / 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 Chromedriver, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- 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: 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
🔇 Additional comments (3)
src/Sylius/Bundle/AdminBundle/Resources/assets/entrypoint.js (1)
16-16: LGTM!The import follows the established pattern for script imports.
src/Sylius/Bundle/AdminBundle/templates/order/show/content/sections/items.html.twig (1)
8-8: Verify icon availability.Ensure that the 'tabler:maximize' icon is available in the icon system.
✅ Verification successful
Icon usage is consistent with the system configuration
The
tabler:maximizeicon follows the established pattern used throughout the codebase, and the icon system is properly configured via UX icons package with extensive tabler icon usage across templates.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for icon definition or usage rg -l "tabler:maximize|tabler/maximize"Length of output: 125
Script:
#!/bin/bash # Search for tabler configuration and other tabler icon usages rg -l "tabler:" --type=twig --type=yaml --type=php # Look for icon system documentation or configuration fd -e md -e yaml -e php -e xml . | xargs rg -l "icon|ux_icon|UxIcon"Length of output: 12424
Script:
#!/bin/bash # Check UX icons configuration cat src/Sylius/Bundle/UiBundle/Resources/config/packages/ux_icons.yamlLength of output: 135
src/Sylius/Bundle/AdminBundle/Resources/translations/messages.en.yml (1)
77-77: LGTM!The translation entry follows the established pattern and is correctly placed in the file.
| <div class="btn-action cursor-pointer" data-fullscreen-btn="orders" data-bs-toggle="tooltip" data-bs-title="{{ 'sylius.ui.full_screen'|trans }}"> | ||
| {{ ux_icon('tabler:maximize') }} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard support for fullscreen toggle.
The fullscreen button should be keyboard accessible and have appropriate ARIA attributes.
-<div class="btn-action cursor-pointer" data-fullscreen-btn="orders" data-bs-toggle="tooltip" data-bs-title="{{ 'sylius.ui.full_screen'|trans }}">
+<button type="button" class="btn-action" data-fullscreen-btn="orders" data-bs-toggle="tooltip" data-bs-title="{{ 'sylius.ui.full_screen'|trans }}" aria-label="{{ 'sylius.ui.full_screen'|trans }}">
{{ ux_icon('tabler:maximize') }}
-</div>
+</button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="btn-action cursor-pointer" data-fullscreen-btn="orders" data-bs-toggle="tooltip" data-bs-title="{{ 'sylius.ui.full_screen'|trans }}"> | |
| {{ ux_icon('tabler:maximize') }} | |
| </div> | |
| <button type="button" class="btn-action" data-fullscreen-btn="orders" data-bs-toggle="tooltip" data-bs-title="{{ 'sylius.ui.full_screen'|trans }}" aria-label="{{ 'sylius.ui.full_screen'|trans }}"> | |
| {{ ux_icon('tabler:maximize') }} | |
| </button> |
| document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => { | ||
| btn.addEventListener('click', () => { | ||
| const containerId = btn.getAttribute('data-fullscreen-btn'); | ||
| const container = document.querySelector(`[data-fullscreen-container="${containerId}"]`); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks and error handling for container selection.
The container selection lacks error handling for cases where the container is not found.
document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => {
btn.addEventListener('click', () => {
const containerId = btn.getAttribute('data-fullscreen-btn');
const container = document.querySelector(`[data-fullscreen-container="${containerId}"]`);
+
+ if (!container) {
+ console.error(`Fullscreen container with ID "${containerId}" not found`);
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => { | |
| btn.addEventListener('click', () => { | |
| const containerId = btn.getAttribute('data-fullscreen-btn'); | |
| const container = document.querySelector(`[data-fullscreen-container="${containerId}"]`); | |
| document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => { | |
| btn.addEventListener('click', () => { | |
| const containerId = btn.getAttribute('data-fullscreen-btn'); | |
| const container = document.querySelector(`[data-fullscreen-container="${containerId}"]`); | |
| if (!container) { | |
| console.error(`Fullscreen container with ID "${containerId}" not found`); | |
| return; | |
| } | |
| if (!document.fullscreenElement) { | ||
| if (container.requestFullscreen) { | ||
| container.requestFullscreen(); | ||
| } | ||
| } else { | ||
| if (document.exitFullscreen) { | ||
| document.exitFullscreen(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and browser compatibility checks.
The current implementation lacks error handling and browser compatibility checks. Consider:
- Handling Fullscreen API errors
- Providing feedback for unsupported browsers
- Adding vendor-prefixed methods for broader browser support
if (!document.fullscreenElement) {
- if (container.requestFullscreen) {
- container.requestFullscreen();
+ const requestFullscreen = container.requestFullscreen
+ || container.mozRequestFullScreen
+ || container.webkitRequestFullscreen
+ || container.msRequestFullscreen;
+
+ if (requestFullscreen) {
+ try {
+ requestFullscreen.call(container);
+ } catch (error) {
+ console.error('Error attempting to enable fullscreen:', error);
+ }
+ } else {
+ console.warn('Fullscreen API is not supported by this browser');
}
} else {
- if (document.exitFullscreen) {
- document.exitFullscreen();
+ const exitFullscreen = document.exitFullscreen
+ || document.mozCancelFullScreen
+ || document.webkitExitFullscreen
+ || document.msExitFullscreen;
+
+ if (exitFullscreen) {
+ try {
+ exitFullscreen.call(document);
+ } catch (error) {
+ console.error('Error attempting to exit fullscreen:', error);
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!document.fullscreenElement) { | |
| if (container.requestFullscreen) { | |
| container.requestFullscreen(); | |
| } | |
| } else { | |
| if (document.exitFullscreen) { | |
| document.exitFullscreen(); | |
| } | |
| } | |
| if (!document.fullscreenElement) { | |
| const requestFullscreen = container.requestFullscreen | |
| || container.mozRequestFullScreen | |
| || container.webkitRequestFullscreen | |
| || container.msRequestFullscreen; | |
| if (requestFullscreen) { | |
| try { | |
| requestFullscreen.call(container); | |
| } catch (error) { | |
| console.error('Error attempting to enable fullscreen:', error); | |
| } | |
| } else { | |
| console.warn('Fullscreen API is not supported by this browser'); | |
| } | |
| } else { | |
| const exitFullscreen = document.exitFullscreen | |
| || document.mozCancelFullScreen | |
| || document.webkitExitFullscreen | |
| || document.msExitFullscreen; | |
| if (exitFullscreen) { | |
| try { | |
| exitFullscreen.call(document); | |
| } catch (error) { | |
| console.error('Error attempting to exit fullscreen:', error); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Sylius/Bundle/AdminBundle/templates/order/show/content/sections/items.html.twig (1)
7-9: Enhance accessibility with ARIA labelsThe fullscreen button should have proper ARIA attributes for better accessibility.
Add ARIA attributes:
- <div class="btn-action cursor-pointer" data-fullscreen-btn="orders" data-bs-toggle="tooltip" data-bs-title="{{ 'sylius.ui.full_screen'|trans }}"> + <button + type="button" + class="btn-action cursor-pointer" + data-fullscreen-btn="orders" + data-bs-toggle="tooltip" + data-bs-title="{{ 'sylius.ui.full_screen'|trans }}" + aria-label="{{ 'sylius.ui.full_screen'|trans }}" + aria-expanded="false" + > {{ ux_icon('tabler:maximize') }} - </div> + </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Sylius/Bundle/AdminBundle/Resources/assets/entrypoint.js(1 hunks)src/Sylius/Bundle/AdminBundle/Resources/assets/scripts/fullscreen.js(1 hunks)src/Sylius/Bundle/AdminBundle/Resources/translations/messages.en.yml(1 hunks)src/Sylius/Bundle/AdminBundle/templates/order/show/content/sections/items.html.twig(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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) / 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 Chromedriver, 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) / 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 Chromedriver, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- 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: 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
🔇 Additional comments (2)
src/Sylius/Bundle/AdminBundle/Resources/assets/entrypoint.js (1)
16-16: LGTM!The import follows the established pattern and is positioned logically among other script imports.
src/Sylius/Bundle/AdminBundle/Resources/translations/messages.en.yml (1)
77-77: LGTM!The translation key follows the established pattern and is appropriately placed in the UI section.
| document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => { | ||
| btn.addEventListener('click', () => { | ||
| const containerId = btn.getAttribute('data-fullscreen-btn'); | ||
| const container = document.querySelector(`[data-fullscreen-container="${containerId}"]`); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard accessibility support
The fullscreen toggle should be accessible via keyboard.
Add keyboard support:
document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => {
+ // Make the button focusable
+ btn.setAttribute('tabindex', '0');
+ btn.setAttribute('role', 'button');
+
+ // Handle both click and keyboard events
btn.addEventListener('click', () => {
const containerId = btn.getAttribute('data-fullscreen-btn');
const container = document.querySelector(`[data-fullscreen-container="${containerId}"]`);
+ });
+
+ btn.addEventListener('keydown', (e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ btn.click();
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => { | |
| btn.addEventListener('click', () => { | |
| const containerId = btn.getAttribute('data-fullscreen-btn'); | |
| const container = document.querySelector(`[data-fullscreen-container="${containerId}"]`); | |
| document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => { | |
| // Make the button focusable | |
| btn.setAttribute('tabindex', '0'); | |
| btn.setAttribute('role', 'button'); | |
| // Handle both click and keyboard events | |
| btn.addEventListener('click', () => { | |
| const containerId = btn.getAttribute('data-fullscreen-btn'); | |
| const container = document.querySelector(`[data-fullscreen-container="${containerId}"]`); | |
| }); | |
| btn.addEventListener('keydown', (e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| btn.click(); | |
| } | |
| }); |
| if (!document.fullscreenElement) { | ||
| if (container.requestFullscreen) { | ||
| container.requestFullscreen(); | ||
| } | ||
| } else { | ||
| if (document.exitFullscreen) { | ||
| document.exitFullscreen(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add browser compatibility checks and error handling
The Fullscreen API implementation should include:
- Browser compatibility checks
- Error handling for failed requests
- Visual feedback for users
Consider this improved implementation:
- if (!document.fullscreenElement) {
- if (container.requestFullscreen) {
- container.requestFullscreen();
- }
- } else {
- if (document.exitFullscreen) {
- document.exitFullscreen();
- }
- }
+ if (!document.fullscreenElement) {
+ try {
+ if (container.requestFullscreen) {
+ await container.requestFullscreen();
+ } else if (container.webkitRequestFullscreen) { // Safari
+ await container.webkitRequestFullscreen();
+ } else if (container.msRequestFullscreen) { // IE11
+ await container.msRequestFullscreen();
+ } else {
+ throw new Error('Fullscreen API not supported');
+ }
+ btn.querySelector('svg').classList.add('rotate-180');
+ } catch (err) {
+ console.error('Failed to enter fullscreen:', err);
+ // Show user-friendly error message
+ }
+ } else {
+ try {
+ if (document.exitFullscreen) {
+ await document.exitFullscreen();
+ } else if (document.webkitExitFullscreen) {
+ await document.webkitExitFullscreen();
+ } else if (document.msExitFullscreen) {
+ await document.msExitFullscreen();
+ }
+ btn.querySelector('svg').classList.remove('rotate-180');
+ } catch (err) {
+ console.error('Failed to exit fullscreen:', err);
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Sylius/Bundle/AdminBundle/templates/order/show/content/sections/items.html.twig (1)
1-1: Consider adding ARIA attributes for accessibility.The fullscreen container should have appropriate ARIA attributes to improve accessibility.
-<div class="card mb-3" data-fullscreen-container="orders"> +<div class="card mb-3" data-fullscreen-container="orders" aria-label="{{ 'sylius.ui.items'|trans }}" role="region">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Sylius/Bundle/AdminBundle/Resources/assets/entrypoint.js(1 hunks)src/Sylius/Bundle/AdminBundle/Resources/assets/scripts/fullscreen.js(1 hunks)src/Sylius/Bundle/AdminBundle/Resources/translations/messages.en.yml(1 hunks)src/Sylius/Bundle/AdminBundle/templates/order/show/content/sections/items.html.twig(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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) / 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 Chromedriver, 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) / 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 Chromedriver, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- 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: 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
🔇 Additional comments (2)
src/Sylius/Bundle/AdminBundle/Resources/assets/entrypoint.js (1)
16-16: LGTM!The fullscreen script import is correctly placed alongside other script imports.
src/Sylius/Bundle/AdminBundle/Resources/translations/messages.en.yml (1)
77-77: LGTM!The translation key is properly added and follows the existing structure.
| <div class="btn-action cursor-pointer" data-fullscreen-btn="orders" data-bs-toggle="tooltip" data-bs-title="{{ 'sylius.ui.full_screen'|trans }}"> | ||
| {{ ux_icon('tabler:maximize') }} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance fullscreen button accessibility and UX.
The fullscreen button should:
- Have keyboard support
- Include ARIA attributes
- Update icon when fullscreen state changes
- <div class="btn-action cursor-pointer" data-fullscreen-btn="orders" data-bs-toggle="tooltip" data-bs-title="{{ 'sylius.ui.full_screen'|trans }}">
- {{ ux_icon('tabler:maximize') }}
- </div>
+ <button type="button" class="btn-action" data-fullscreen-btn="orders" data-bs-toggle="tooltip" data-bs-title="{{ 'sylius.ui.full_screen'|trans }}"
+ aria-label="{{ 'sylius.ui.full_screen'|trans }}" aria-expanded="false">
+ <span class="fullscreen-icon">{{ ux_icon('tabler:maximize') }}</span>
+ <span class="exit-fullscreen-icon d-none">{{ ux_icon('tabler:minimize') }}</span>
+ </button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="btn-action cursor-pointer" data-fullscreen-btn="orders" data-bs-toggle="tooltip" data-bs-title="{{ 'sylius.ui.full_screen'|trans }}"> | |
| {{ ux_icon('tabler:maximize') }} | |
| </div> | |
| <button type="button" class="btn-action" data-fullscreen-btn="orders" data-bs-toggle="tooltip" data-bs-title="{{ 'sylius.ui.full_screen'|trans }}" | |
| aria-label="{{ 'sylius.ui.full_screen'|trans }}" aria-expanded="false"> | |
| <span class="fullscreen-icon">{{ ux_icon('tabler:maximize') }}</span> | |
| <span class="exit-fullscreen-icon d-none">{{ ux_icon('tabler:minimize') }}</span> | |
| </button> |
| document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => { | ||
| btn.addEventListener('click', () => { | ||
| const containerId = btn.getAttribute('data-fullscreen-btn'); | ||
| const container = document.querySelector(`[data-fullscreen-container="${containerId}"]`); | ||
|
|
||
| if (!document.fullscreenElement) { | ||
| if (container.requestFullscreen) { | ||
| container.requestFullscreen(); | ||
| } | ||
| } else { | ||
| if (document.exitFullscreen) { | ||
| document.exitFullscreen(); | ||
| } | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance fullscreen implementation with proper error handling and state management.
The current implementation needs improvements for better browser support, error handling, and user experience.
Here's a more robust implementation:
-document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => {
- btn.addEventListener('click', () => {
- const containerId = btn.getAttribute('data-fullscreen-btn');
- const container = document.querySelector(`[data-fullscreen-container="${containerId}"]`);
-
- if (!document.fullscreenElement) {
- if (container.requestFullscreen) {
- container.requestFullscreen();
- }
- } else {
- if (document.exitFullscreen) {
- document.exitFullscreen();
- }
- }
- });
-});
+// Check if fullscreen is supported
+const fullscreenEnabled = document.fullscreenEnabled ||
+ document.webkitFullscreenEnabled ||
+ document.mozFullScreenEnabled ||
+ document.msFullscreenEnabled;
+
+function getFullscreenElement() {
+ return document.fullscreenElement ||
+ document.webkitFullscreenElement ||
+ document.mozFullScreenElement ||
+ document.msFullscreenElement;
+}
+
+function requestFullscreen(element) {
+ if (element.requestFullscreen) {
+ return element.requestFullscreen();
+ } else if (element.webkitRequestFullscreen) {
+ return element.webkitRequestFullscreen();
+ } else if (element.mozRequestFullScreen) {
+ return element.mozRequestFullScreen();
+ } else if (element.msRequestFullscreen) {
+ return element.msRequestFullscreen();
+ }
+}
+
+function exitFullscreen() {
+ if (document.exitFullscreen) {
+ return document.exitFullscreen();
+ } else if (document.webkitExitFullscreen) {
+ return document.webkitExitFullscreen();
+ } else if (document.mozCancelFullScreen) {
+ return document.mozCancelFullScreen();
+ } else if (document.msExitFullscreen) {
+ return document.msExitFullscreen();
+ }
+}
+
+function updateFullscreenButton(button, isFullscreen) {
+ const maximizeIcon = button.querySelector('.fullscreen-icon');
+ const minimizeIcon = button.querySelector('.exit-fullscreen-icon');
+
+ maximizeIcon.classList.toggle('d-none', isFullscreen);
+ minimizeIcon.classList.toggle('d-none', !isFullscreen);
+ button.setAttribute('aria-expanded', isFullscreen);
+}
+
+if (fullscreenEnabled) {
+ document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => {
+ btn.addEventListener('click', async () => {
+ try {
+ const containerId = btn.getAttribute('data-fullscreen-btn');
+ const container = document.querySelector(`[data-fullscreen-container="${containerId}"]`);
+
+ if (!container) {
+ throw new Error(`Container with id "${containerId}" not found`);
+ }
+
+ if (!getFullscreenElement()) {
+ await requestFullscreen(container);
+ } else {
+ await exitFullscreen();
+ }
+ } catch (error) {
+ console.error('Fullscreen error:', error);
+ // Optionally show user-friendly error message
+ }
+ });
+
+ // Update button state when fullscreen changes
+ document.addEventListener('fullscreenchange', () => {
+ updateFullscreenButton(btn, !!getFullscreenElement());
+ });
+ });
+} else {
+ // Hide fullscreen buttons if not supported
+ document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => {
+ btn.style.display = 'none';
+ });
+}Key improvements:
- Cross-browser compatibility
- Error handling
- Button state management
- Fullscreen change event handling
- Graceful degradation when fullscreen is not supported
Committable suggestion skipped: line range outside the PR's diff.
fed9811 to
44a274e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Sylius/Bundle/AdminBundle/Resources/assets/scripts/fullscreen.js (2)
10-14: 🛠️ Refactor suggestionAdd keyboard accessibility support.
The fullscreen toggle should be accessible via keyboard for better accessibility compliance.
The implementation should include keyboard support as suggested in the past review.
20-28: 🛠️ Refactor suggestionEnhance browser compatibility and error handling.
The current implementation needs improvements for:
- Cross-browser support with vendor prefixes
- Error handling for failed fullscreen requests
- Visual feedback for state changes
Consider implementing the enhanced version from past reviews that includes proper error handling and browser compatibility checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Sylius/Bundle/AdminBundle/Resources/assets/entrypoint.js(1 hunks)src/Sylius/Bundle/AdminBundle/Resources/assets/scripts/fullscreen.js(1 hunks)src/Sylius/Bundle/AdminBundle/Resources/translations/messages.en.yml(1 hunks)src/Sylius/Bundle/AdminBundle/templates/order/show/content/sections/items.html.twig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Sylius/Bundle/AdminBundle/Resources/assets/entrypoint.js
- src/Sylius/Bundle/AdminBundle/Resources/translations/messages.en.yml
- src/Sylius/Bundle/AdminBundle/templates/order/show/content/sections/items.html.twig
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Static checks / PHP 8.3, Symfony ^7.1
- GitHub Check: Static checks / PHP 8.2, Symfony ^6.4
🔇 Additional comments (2)
src/Sylius/Bundle/AdminBundle/Resources/assets/scripts/fullscreen.js (2)
1-9: LGTM!The license header is properly formatted and includes all necessary information.
15-19: LGTM!The error handling for missing container is properly implemented.
| document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => { | ||
| btn.addEventListener('click', () => { | ||
| const containerId = btn.getAttribute('data-fullscreen-btn'); | ||
| const container = document.querySelector(`[data-fullscreen-container="${containerId}"]`); | ||
|
|
||
| if (!container) { | ||
| console.error(`Fullscreen container with ID "${containerId}" not found`); | ||
| return; | ||
| } | ||
|
|
||
| if (!document.fullscreenElement) { | ||
| if (container.requestFullscreen) { | ||
| container.requestFullscreen(); | ||
| } | ||
| } else { | ||
| if (document.exitFullscreen) { | ||
| document.exitFullscreen(); | ||
| } | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add state management and event handling.
The implementation is missing:
- Event listener for fullscreenchange events
- Visual feedback for current state
- Proper cleanup of event listeners
Add state management:
document.querySelectorAll('[data-fullscreen-btn]').forEach(btn => {
+ // Handle fullscreen change events
+ const handleFullscreenChange = () => {
+ const isFullscreen = !!document.fullscreenElement;
+ btn.setAttribute('aria-expanded', isFullscreen);
+ // Update icon/visual state
+ btn.classList.toggle('is-fullscreen', isFullscreen);
+ };
+
+ document.addEventListener('fullscreenchange', handleFullscreenChange);
+
btn.addEventListener('click', () => {
const containerId = btn.getAttribute('data-fullscreen-btn');
const container = document.querySelector(`[data-fullscreen-container="${containerId}"]`);
@@ -28,3 +37,8 @@
}
});
});
+
+// Cleanup on page unload
+window.addEventListener('unload', () => {
+ document.removeEventListener('fullscreenchange', handleFullscreenChange);
+});Committable suggestion skipped: line range outside the PR's diff.
In many cases, the order table is compressed and difficult to read. This PR adds a feature that allows users to open the table in fullscreen mode, making more products visible and improving readability.
Preview
fullscreen.mp4
Summary by CodeRabbit
New Features
Documentation
Style