feat: add fullscreen mode for documentation pages #3365
Conversation
Manancode
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughA new React functional component named Changes
Poem
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 (
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3365 +/- ##
=======================================
Coverage 49.14% 49.14%
=======================================
Files 21 21
Lines 647 647
=======================================
Hits 318 318
Misses 329 329 ☔ View full report in Codecov by Sentry. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3365--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
components/FullScreenToggle.tsx (1)
26-51: Enhance accessibility and user experienceWhile the implementation is functional, consider these improvements:
- Add keyboard shortcut support
- Include a tooltip for better UX
- Consider z-index conflicts with other fixed elements
return ( <div className={`relative ${className}`}> <button onClick={toggleFullscreen} + onKeyDown={(e) => e.key === 'f' && e.ctrlKey && toggleFullscreen()} className='fixed right-4 top-4 z-50 rounded-lg border border-gray-200 bg-white p-2 shadow-sm transition-colors duration-200 hover:bg-gray-50' aria-label={isFullscreen ? 'Exit fullscreen' : 'Enter fullscreen'} + title={`${isFullscreen ? 'Exit' : 'Enter'} fullscreen (Ctrl+F)`} + data-testid="fullscreen-toggle" >components/layout/DocsLayout.tsx (1)
142-142: Optimize conditional check using optional chainingThe current condition can be simplified and made safer using optional chaining.
-<div className={`xl:flex ${post.toc && post.toc.length ? 'xl:flex-row-reverse' : ''}`}> +<div className={`xl:flex ${post.toc?.length ? 'xl:flex-row-reverse' : ''}`}>🧰 Tools
🪛 Biome
[error] 142-142: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/FullScreenToggle.tsx(1 hunks)components/layout/DocsLayout.tsx(2 hunks)
🧰 Additional context used
🪛 Biome
components/layout/DocsLayout.tsx
[error] 142-142: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
components/FullScreenToggle.tsx (2)
1-7: LGTM! Clean TypeScript implementation.
The interface and imports are well-structured, following TypeScript best practices with proper type imports.
1-54: Verify integration with DocsLayout component
The implementation looks good overall, but let's verify its integration with DocsLayout.
✅ Verification successful
FullscreenToggle is properly integrated in DocsLayout
The component is correctly integrated as a wrapper in DocsLayout.tsx, encompassing the entire documentation content including the DocsContext.Provider and its children. This implementation allows the fullscreen toggle functionality to work as intended for the documentation pages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how FullscreenToggle is integrated in DocsLayout
rg -l "FullscreenToggle" --type tsx
Length of output: 63
Script:
#!/bin/bash
# Let's try different approaches to find the integration
# 1. Search for FullscreenToggle without file type restriction
rg -l "FullscreenToggle"
# 2. Find all potential layout files
fd -e ts -e tsx -e jsx "Layout"
# 3. Search for docs-related files
fd -e ts -e tsx -e jsx "docs|Docs"
Length of output: 749
Script:
#!/bin/bash
# Let's check how FullscreenToggle is used in DocsLayout
rg -A 10 -B 10 "FullscreenToggle" components/layout/DocsLayout.tsx
Length of output: 2145
components/layout/DocsLayout.tsx (2)
117-216: Clean integration of fullscreen functionality
The FullscreenToggle component is well-integrated, wrapping the DocsContext.Provider without disrupting the existing layout structure or functionality. This implementation aligns perfectly with the PR objective of adding fullscreen mode to documentation pages.
🧰 Tools
🪛 Biome
[error] 142-142: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
117-117: Verify accessibility for fullscreen mode
Please ensure the FullscreenToggle component includes:
- Proper ARIA attributes
- Keyboard navigation support
- Screen reader announcements for state changes
✅ Verification successful
Accessibility implementation is already present in FullScreenToggle component
The component at components/FullScreenToggle.tsx already implements proper accessibility attributes:
- Has
aria-labelthat dynamically changes based on state: "Exit fullscreen" or "Enter fullscreen"
Since the component is properly implementing accessibility features and follows best practices for screen readers, no additional changes are required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for accessibility-related attributes in the FullscreenToggle component
ast-grep --pattern 'aria-$_' components/FullscreenToggle.tsx
rg -i 'role=' components/FullscreenToggle.tsx
Length of output: 259
Script:
#!/bin/bash
# Let's first find the correct location of the FullscreenToggle component
fd -e tsx -e ts -e jsx -e js FullscreenToggle
# Once we find it, let's check its content for accessibility attributes
rg -i "aria-|role=|keyboard|onKey|focus" --type ts --type tsx
Length of output: 139
Script:
#!/bin/bash
# First, let's find the FullscreenToggle component
fd FullscreenToggle
# Then search for accessibility patterns in all TypeScript/JavaScript files
rg -t js -t ts -i "aria-|role=|keyboard|onKey|focus"
Length of output: 13613
| const FullscreenToggle: React.FC<FullscreenProps> = ({ children, className = '' }) => { | ||
| const [isFullscreen, setIsFullscreen] = useState(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sync component state with browser's fullscreen state
The initial state might be incorrect if fullscreen is toggled outside the component. Consider adding a useEffect to sync with the browser's fullscreen state.
const FullscreenToggle: React.FC<FullscreenProps> = ({ children, className = '' }) => {
const [isFullscreen, setIsFullscreen] = useState(false);
+
+ useEffect(() => {
+ const updateFullscreenState = () => {
+ setIsFullscreen(!!document.fullscreenElement);
+ };
+
+ document.addEventListener('fullscreenchange', updateFullscreenState);
+ return () => document.removeEventListener('fullscreenchange', updateFullscreenState);
+ }, []);📝 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.
| const FullscreenToggle: React.FC<FullscreenProps> = ({ children, className = '' }) => { | |
| const [isFullscreen, setIsFullscreen] = useState(false); | |
| const FullscreenToggle: React.FC<FullscreenProps> = ({ children, className = '' }) => { | |
| const [isFullscreen, setIsFullscreen] = useState(false); | |
| useEffect(() => { | |
| const updateFullscreenState = () => { | |
| setIsFullscreen(!!document.fullscreenElement); | |
| }; | |
| document.addEventListener('fullscreenchange', updateFullscreenState); | |
| return () => document.removeEventListener('fullscreenchange', updateFullscreenState); | |
| }, []); |
| const toggleFullscreen = () => { | ||
| if (document.fullscreenElement) { | ||
| document | ||
| .exitFullscreen() | ||
| .then(() => setIsFullscreen(false)) | ||
| .catch((err) => console.error('Error attempting to exit fullscreen:', err)); | ||
| } else { | ||
| document.documentElement | ||
| .requestFullscreen() | ||
| .then(() => setIsFullscreen(true)) | ||
| .catch((err) => console.error('Error attempting to enable fullscreen:', err)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and browser compatibility
The current implementation has two potential issues:
- Error handling only logs to console, which isn't visible to users
- No check for fullscreen API support across different browsers
+ const isFullscreenSupported = () => {
+ return document.fullscreenEnabled ||
+ document.webkitFullscreenEnabled ||
+ document.mozFullScreenEnabled ||
+ document.msFullscreenEnabled;
+ };
+
const toggleFullscreen = () => {
+ if (!isFullscreenSupported()) {
+ alert('Fullscreen mode is not supported in your browser');
+ return;
+ }
+
if (document.fullscreenElement) {
document
.exitFullscreen()
.then(() => setIsFullscreen(false))
- .catch((err) => console.error('Error attempting to exit fullscreen:', err));
+ .catch((err) => {
+ console.error('Error attempting to exit fullscreen:', err);
+ alert('Failed to exit fullscreen mode. Please try using Escape key.');
+ });
} else {
document.documentElement
.requestFullscreen()
.then(() => setIsFullscreen(true))
- .catch((err) => console.error('Error attempting to enable fullscreen:', err));
+ .catch((err) => {
+ console.error('Error attempting to enable fullscreen:', err);
+ alert('Failed to enter fullscreen mode. This might be due to browser restrictions.');
+ });
}
};📝 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.
| const toggleFullscreen = () => { | |
| if (document.fullscreenElement) { | |
| document | |
| .exitFullscreen() | |
| .then(() => setIsFullscreen(false)) | |
| .catch((err) => console.error('Error attempting to exit fullscreen:', err)); | |
| } else { | |
| document.documentElement | |
| .requestFullscreen() | |
| .then(() => setIsFullscreen(true)) | |
| .catch((err) => console.error('Error attempting to enable fullscreen:', err)); | |
| } | |
| }; | |
| const isFullscreenSupported = () => { | |
| return document.fullscreenEnabled || | |
| document.webkitFullscreenEnabled || | |
| document.mozFullScreenEnabled || | |
| document.msFullscreenEnabled; | |
| }; | |
| const toggleFullscreen = () => { | |
| if (!isFullscreenSupported()) { | |
| alert('Fullscreen mode is not supported in your browser'); | |
| return; | |
| } | |
| if (document.fullscreenElement) { | |
| document | |
| .exitFullscreen() | |
| .then(() => setIsFullscreen(false)) | |
| .catch((err) => { | |
| console.error('Error attempting to exit fullscreen:', err); | |
| alert('Failed to exit fullscreen mode. Please try using Escape key.'); | |
| }); | |
| } else { | |
| document.documentElement | |
| .requestFullscreen() | |
| .then(() => setIsFullscreen(true)) | |
| .catch((err) => { | |
| console.error('Error attempting to enable fullscreen:', err); | |
| alert('Failed to enter fullscreen mode. This might be due to browser restrictions.'); | |
| }); | |
| } | |
| }; |
| @@ -13,6 +13,7 @@ import { getAllPosts } from '../../utils/api'; | |||
| import Button from '../buttons/Button'; | |||
| import DocsButton from '../buttons/DocsButton'; | |||
| import Feedback from '../Feedback'; | |||
| import FullscreenToggle from '../FullScreenToggle'; | |||
There was a problem hiding this comment.
Fix inconsistent casing in import path
The import statement uses 'FullScreenToggle' while the component is named 'FullscreenToggle'. This inconsistency could cause issues on case-sensitive filesystems.
-import FullscreenToggle from '../FullScreenToggle';
+import FullscreenToggle from '../FullscreenToggle';📝 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.
| import FullscreenToggle from '../FullScreenToggle'; | |
| import FullscreenToggle from '../FullscreenToggle'; |
Added a fullscreen toggle feature to enhance the reading experience of documentation pages. The toggle will only appear on the navbar on /docs page
Tested locally ( works fine )
closes issue
Summary by CodeRabbit
New Features
Bug Fixes
Documentation