fix: improve mobile UX for Specification Explorer with content-first layout and toggle#5107
fix: improve mobile UX for Specification Explorer with content-first layout and toggle#5107Sourya07 wants to merge 9 commits intoasyncapi:masterfrom
Conversation
Sourya07
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
princerajpoot20 and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds a mobile-toggle to the Visualizer for showing/hiding the visual map, responsive CSS media queries to adjust visualizer layout on small screens, and hides the DocsLayout Menu button on small viewports; no configuration semantics or exported APIs changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5107 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 796 796
Branches 146 146
=========================================
Hits 796 796 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5107--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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/layout/DocsLayout.tsx (1)
100-114:⚠️ Potential issue | 🟠 MajorHiding the Menu button removes sidebar navigation access on mobile.
hidden md:blockcompletely removes the only way to open the explorer sidebar on small screens. The newShow Visual Maptoggle inVisualizer.tsxcontrols the node-container visibility, not the sidebar navigation. Mobile users lose the ability to navigate between spec sections.Consider replacing
hiddenwith a mobile-friendly alternative (e.g., a collapsible menu or incorporating a sidebar trigger into the mobile toggle area) rather than removing it entirely.
🤖 Fix all issues with AI agents
In `@components/docs/Visualizer.tsx`:
- Around line 19-26: The wrapper div uses class "mobile-toggle" which is hidden
by a custom CSS breakpoint at min-width:769px, while the button inside also has
Tailwind's md:hidden (min-width:768px), causing a mismatch at 768px; to fix,
remove the Tailwind breakpoint class md:hidden from the button element in
Visualizer.tsx (the button that toggles setShowMap using showMap) so visibility
is controlled solely by the .mobile-toggle rule in styles/globals.css, or
alternatively remove/adjust the .mobile-toggle rule to match Tailwind's md
breakpoint—pick one approach and make both use the same breakpoint.
In `@styles/globals.css`:
- Around line 359-368: The .visualizer-container .node-container rule uses
width: 125% which causes horizontal overflow despite transform: scale(0.8);
change the layout to avoid overflow by setting width: 100% on
.visualizer-container .node-container (or a more appropriate max-width) and rely
on transform: scale(...) for visual shrinkage, or alternatively wrap
.node-container in a parent with overflow: hidden and keep width as-is; update
the CSS rule for .visualizer-container .node-container (and any container markup
if adding a wrapper) to prevent horizontal scrollbars while preserving the
visual scale and transform-origin: top left.
🧹 Nitpick comments (2)
components/layout/DocsLayout.tsx (1)
105-110: Simplify the toggle.
if/elsethat sets a boolean totrue/falseis equivalent to toggling.Proposed fix
onClick={() => { - if (explorerDocMenu) { - setExplorerDocMenu(false); - } else { - setExplorerDocMenu(true); - } + setExplorerDocMenu((prev) => !prev); }}styles/globals.css (1)
350-401: Heavy!importantusage — consider scoping or specificity instead.Nearly every declaration uses
!important, likely to override Schyma's inline styles. This is understandable but fragile — any future Schyma update or refactor could break these overrides silently. If Schyma applies inline styles,!importantmay be the only option, but it's worth documenting that dependency (a brief comment above the media query would suffice).Also, the mobile breakpoint at
768pxand the desktop breakpoint at769pxdon't align with Tailwind'smdbreakpoint (min-width: 768px). Consider using767px/768pxto match Tailwind conventions, or stick entirely to Tailwind utilities.
Sourya07
changed the title
Sourya07
changed the title
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/docs/Visualizer.tsx`:
- Around line 14-26: The toggle button in the Visualizer component should expose
its expanded state to assistive tech: update the button (the one using
onClick={() => setShowMap(!showMap)} in Visualizer) to include
aria-expanded={showMap} (and optionally aria-controls referencing the visual map
container's id) so screen readers can announce whether the visual map is shown;
keep the existing setShowMap and showMap logic unchanged.
🧹 Nitpick comments (1)
components/docs/Visualizer.tsx (1)
4-4: UnnecessaryReactdefault import.With React 18's automatic JSX transform, the
Reactdefault import is not needed — only{ useState }is required. Minor nit, no functional impact.Suggested tweak
-import React, { useState } from 'react'; +import { useState } from 'react';
| function Visualizer() { | ||
| const [showMap, setShowMap] = useState(false); | ||
|
|
||
| return ( | ||
| <div> | ||
| <div className={`visualizer-container ${showMap ? 'show-map' : ''}`}> | ||
| <div className='mobile-toggle px-4'> | ||
| <button | ||
| onClick={() => setShowMap(!showMap)} | ||
| className='bg-primary-500 hover:bg-primary-600 text-white font-bold py-2 px-4 rounded mb-2 mt-4 w-full' | ||
| > | ||
| {showMap ? 'Hide Visual Map' : 'Show Visual Map'} | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
Add aria-expanded to the toggle button for accessibility.
The toggle controls visibility of the visual map but lacks aria-expanded, which screen readers rely on to convey the current state. Given the PR's Lighthouse accessibility score of 98, this small addition would help maintain that standard.
♿ Proposed fix
<button
+ aria-expanded={showMap}
onClick={() => setShowMap(!showMap)}
className='bg-primary-500 hover:bg-primary-600 text-white font-bold py-2 px-4 rounded mb-2 mt-4 w-full'
>📝 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.
| function Visualizer() { | |
| const [showMap, setShowMap] = useState(false); | |
| return ( | |
| <div> | |
| <div className={`visualizer-container ${showMap ? 'show-map' : ''}`}> | |
| <div className='mobile-toggle px-4'> | |
| <button | |
| onClick={() => setShowMap(!showMap)} | |
| className='bg-primary-500 hover:bg-primary-600 text-white font-bold py-2 px-4 rounded mb-2 mt-4 w-full' | |
| > | |
| {showMap ? 'Hide Visual Map' : 'Show Visual Map'} | |
| </button> | |
| </div> | |
| function Visualizer() { | |
| const [showMap, setShowMap] = useState(false); | |
| return ( | |
| <div className={`visualizer-container ${showMap ? 'show-map' : ''}`}> | |
| <div className='mobile-toggle px-4'> | |
| <button | |
| aria-expanded={showMap} | |
| onClick={() => setShowMap(!showMap)} | |
| className='bg-primary-500 hover:bg-primary-600 text-white font-bold py-2 px-4 rounded mb-2 mt-4 w-full' | |
| > | |
| {showMap ? 'Hide Visual Map' : 'Show Visual Map'} | |
| </button> | |
| </div> |
🤖 Prompt for AI Agents
In `@components/docs/Visualizer.tsx` around lines 14 - 26, The toggle button in
the Visualizer component should expose its expanded state to assistive tech:
update the button (the one using onClick={() => setShowMap(!showMap)} in
Visualizer) to include aria-expanded={showMap} (and optionally aria-controls
referencing the visual map container's id) so screen readers can announce
whether the visual map is shown; keep the existing setShowMap and showMap logic
unchanged.
|
@princerajpoot20 please take a look 👍. |
3 participants
Description
fixes the issue for the Specification Explorer
->#4757
by making changes in the ->
Related issue(s)
Summary by CodeRabbit
New Features
Style