Skip to content

Fullscreen order table view#17620

Merged
GSadee merged 1 commit intoSylius:2.0from
kulczy:SYL-4237/fullscreen-orders
Jan 16, 2025
Merged

Fullscreen order table view#17620
GSadee merged 1 commit intoSylius:2.0from
kulczy:SYL-4237/fullscreen-orders

Conversation

@kulczy
Copy link
Copy Markdown
Contributor

@kulczy kulczy commented Jan 10, 2025

Q A
Branch? 2.0
Bug fix? no
New feature? yes
BC breaks? no
License MIT

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

    • Added fullscreen functionality for order items section
    • Introduced a button to toggle fullscreen mode for specific elements
    • Implemented a mechanism to handle fullscreen mode for designated elements
  • Documentation

    • Added English translation for "Full screen" feature
  • Style

    • Updated card header layout to include fullscreen toggle button
    • Enhanced functionality with new attributes for fullscreen display

@kulczy kulczy requested review from a team as code owners January 10, 2025 12:52
@probot-autolabeler probot-autolabeler bot added the Admin AdminBundle related issues and PRs. label Jan 10, 2025
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jan 10, 2025

Walkthrough

This 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

File Change Summary
src/Sylius/Bundle/AdminBundle/Resources/assets/entrypoint.js Added import for ./scripts/fullscreen script
src/Sylius/Bundle/AdminBundle/Resources/assets/scripts/fullscreen.js New script to handle fullscreen toggling for elements with data-fullscreen-btn attribute
src/Sylius/Bundle/AdminBundle/Resources/translations/messages.en.yml Added ui.full_screen translation key with value 'Full screen'
src/Sylius/Bundle/AdminBundle/templates/order/show/content/sections/items.html.twig Added data-fullscreen-container="orders" attribute and fullscreen toggle button to card

Possibly related PRs

Suggested reviewers

  • GSadee

Poem

🐰 A Rabbit's Ode to Fullscreen Delight
With just one click, the screen takes flight,
Expanding views both wide and bright,
No more squinting, no more strain,
Full screen magic breaks the chain!
🖥️ Hop into clarity, pure and light! 🐇

Finishing Touches

  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@kulczy kulczy changed the title Full-Screen Order Table View Fullscreen order table view Jan 10, 2025
@GSadee GSadee added the UX Issues and PRs aimed at improving User eXperience. label Jan 10, 2025
GSadee
GSadee previously approved these changes Jan 10, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 10, 2025

Bunnyshell Preview Environment deleted

Available commands:

  • /bns:deploy to redeploy the environment

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 9e7f1bf and fed9811.

📒 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:maximize icon 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.yaml

Length 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.

Comment on lines +7 to +9
<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>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
<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>

Comment on lines +10 to +14
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}"]`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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;
}

Comment on lines +15 to +28
if (!document.fullscreenElement) {
if (container.requestFullscreen) {
container.requestFullscreen();
}
} else {
if (document.exitFullscreen) {
document.exitFullscreen();
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling and browser compatibility checks.

The current implementation lacks error handling and browser compatibility checks. Consider:

  1. Handling Fullscreen API errors
  2. Providing feedback for unsupported browsers
  3. 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.

Suggested change
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);
}
}
}

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 labels

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9e7f1bf and fed9811.

📒 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.

Comment on lines +10 to +14
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}"]`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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();
}
});

Comment on lines +15 to +28
if (!document.fullscreenElement) {
if (container.requestFullscreen) {
container.requestFullscreen();
}
} else {
if (document.exitFullscreen) {
document.exitFullscreen();
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add browser compatibility checks and error handling

The Fullscreen API implementation should include:

  1. Browser compatibility checks
  2. Error handling for failed requests
  3. 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.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 9e7f1bf and fed9811.

📒 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.

Comment on lines +7 to +9
<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>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance fullscreen button accessibility and UX.

The fullscreen button should:

  1. Have keyboard support
  2. Include ARIA attributes
  3. 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.

Suggested change
<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>

Comment on lines +10 to +30
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();
}
}
});
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Cross-browser compatibility
  2. Error handling
  3. Button state management
  4. Fullscreen change event handling
  5. Graceful degradation when fullscreen is not supported

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
src/Sylius/Bundle/AdminBundle/Resources/assets/scripts/fullscreen.js (2)

10-14: 🛠️ Refactor suggestion

Add 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 suggestion

Enhance browser compatibility and error handling.

The current implementation needs improvements for:

  1. Cross-browser support with vendor prefixes
  2. Error handling for failed fullscreen requests
  3. 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

📥 Commits

Reviewing files that changed from the base of the PR and between fed9811 and 44a274e.

📒 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.

Comment on lines +10 to +30
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();
}
}
});
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add state management and event handling.

The implementation is missing:

  1. Event listener for fullscreenchange events
  2. Visual feedback for current state
  3. 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.

@GSadee GSadee merged commit d18ef06 into Sylius:2.0 Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Admin AdminBundle related issues and PRs. UX Issues and PRs aimed at improving User eXperience.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants