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 10 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a mobile toggle in the Visualizer to show/hide the visual map, responsive CSS 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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
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 830 830
Branches 159 159
=========================================
Hits 830 830 ☔ 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 👍. |
|
@princerajpoot20 please take a look |
|
@princerajpoot20 thanks |
|
|
@Sourya07 Can you please create another PR where we need to disable the spec explorer for mobile devices by showing a not available screen. Please create another issue ticket for the same. |
4 participants
Description
fixes the issue for the Specification Explorer
->#4757
by making changes in the ->
Related issue(s)
Summary by CodeRabbit
New Features
Style