feat: added the toggle button so that reader can read the whole docs … #3328
Conversation
mohdshubair313
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThis pull request introduces two new functional components, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 Biomestyles/globals.css[error] 359-359: Unknown property is not allowed. See CSS Specifications and browser specific properties for more details. (lint/correctness/noUnknownProperty) [error] 363-363: Unknown property is not allowed. See CSS Specifications and browser specific properties for more details. (lint/correctness/noUnknownProperty) 🔇 Additional comments (2)styles/globals.css (2)
The previous review comment suggesting UX improvements for the button styling is still valid for this implementation. 🧰 Tools🪛 Biome[error] 359-359: Unknown property is not allowed. See CSS Specifications and browser specific properties for more details. (lint/correctness/noUnknownProperty)
Fix CSS syntax errors and enhance accessibility. There are several issues with the current implementation:
Apply this diff to fix the issues and enhance the implementation: button#sidebarToggle {
width: 50px;
height: 50px;
display: inline-block;
- backgroundColor: white;
+ background-color: white;
+ border: 1px solid #e4e4e4;
+ border-radius: 4px;
+ cursor: pointer;
+ transition: all 0.2s ease;
+ position: relative;
+
+ /* Ensure the button is accessible */
+ &:focus-visible {
+ outline: 2px solid #4b79cf;
+ outline-offset: 2px;
+ }
+
+ &:hover {
+ background-color: #f5f5f5;
+ }
}
button#sidebarToggle.open {
- backgroundcolor: black;
+ background-color: #f5f5f5;
}This implementation:
✅ Verification successfulFix CSS syntax errors and enhance accessibility in The verification confirms that the incorrect CSS property names (
The suggested improvements for accessibility and UX (transitions, focus states, hover effects) will enhance the implementation without affecting other parts of the codebase. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Verify if there are any other instances of incorrect CSS property names
rg -i 'backgroundColor|backgroundcolor' '*.css'
Length of output: 95 Script: #!/bin/bash
# First, let's find all CSS files in the repository
fd -e css
# Then search for incorrect CSS property names in those files
fd -e css --exec rg -i 'backgroundColor|backgroundcolor' {}
Length of output: 194 🧰 Tools🪛 Biome[error] 359-359: Unknown property is not allowed. See CSS Specifications and browser specific properties for more details. (lint/correctness/noUnknownProperty) [error] 363-363: Unknown property is not allowed. See CSS Specifications and browser specific properties for more details. (lint/correctness/noUnknownProperty) 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 (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
❌ Deploy Preview for asyncapi-website failed.Built without sensitive environment variables
|
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3328--asyncapi-website.netlify.app/ |
mohdshubair313
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
styles/globals.css (1)
354-364: Consider performance optimizations for the toggle button.Given the low Lighthouse performance score (47), we should optimize the button's rendering performance:
- Use
transforminstead of background-color for animations- Add
will-changehint when appropriate- Consider using
@layerto better organize styles+@layer components { + .sidebar-toggle { + /* ... existing styles ... */ + transform: translateZ(0); + } + + .sidebar-toggle--open { + /* ... existing styles ... */ + } + + /* Add will-change only when the button is about to animate */ + .sidebar-toggle--animating { + will-change: transform, background-color; + } +}This optimization aligns with the PR's goal of improving the documentation viewing experience while addressing the performance concerns highlighted in the Lighthouse report.
components/layout/DocsLayout.tsx (3)
83-89: Improve state variable naming convention.The state management implementation is correct, but the setter function name
setisSidebarVisibleshould follow camelCase convention.-const [isSidebarVisible, setisSidebarVisible] = useState(true); +const [isSidebarVisible, setIsSidebarVisible] = useState(true);
Line range hint
144-237: Refactor layout structure to avoid code duplication.The current implementation duplicates the main content markup when the sidebar is hidden. This can lead to maintenance issues and potential inconsistencies.
Consider refactoring to use CSS classes for controlling the layout instead:
-{!isSidebarVisible && ( - <div className='flex flex-row' id='main-content'> - <!-- duplicated content --> - </div> -)} +<div className={`flex flex-row transition-all duration-200 ${isSidebarVisible ? 'ml-64' : 'ml-0'}`}> + <!-- single instance of content --> +</div>This approach:
- Eliminates code duplication
- Provides smooth transitions
- Maintains consistent structure
129-143: Address performance implications of sidebar toggle.Given the low performance score (47) in the Lighthouse report, consider these optimizations:
- Add
widthandheightattributes to the icon components to prevent layout shifts- Use CSS transform instead of conditional rendering for smoother transitions
- Consider using
React.memo()for the sidebar component to prevent unnecessary re-rendersExample optimization:
+const MemoizedSidebar = React.memo(DocsNavWrapper); <div className='flex flex-row' id='main-content'> <button id='sidebarToggle' /* ... */ > {isSidebarVisible ? <HideIcon width={24} height={24} /> : <ShowIcon width={24} height={24} />} </button> - {isSidebarVisible && ( - <div className='lg:flex lg:shrink-0'> - {sidebar} - </div> - )} + <div className={`lg:flex lg:shrink-0 transform transition-transform duration-200 ${ + isSidebarVisible ? 'translate-x-0' : '-translate-x-full' + }`}> + <MemoizedSidebar setShowMenu={setShowMenu} navigation={navigation} post={post} /> + </div> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- components/icons/HideIcon.tsx (1 hunks)
- components/icons/ShowIcon.tsx (1 hunks)
- components/layout/DocsLayout.tsx (4 hunks)
- styles/globals.css (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/icons/ShowIcon.tsx
🔇 Additional comments (3)
components/icons/HideIcon.tsx (1)
1-9: Verify the icon's usage in the sidebar toggle functionality.The icon appears to be part of the sidebar toggle feature mentioned in the PR objectives.
Let's verify its implementation:
✅ Verification successful
HideIcon is correctly implemented in the sidebar toggle functionality
The verification confirms that:
- HideIcon is properly imported and used in DocsLayout.tsx
- It's correctly integrated into the sidebar toggle button, switching between HideIcon and ShowIcon based on the
isSidebarVisiblestate- The toggle functionality is properly implemented with state management and conditional rendering
- The button has appropriate accessibility attributes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how the HideIcon is imported and used in the DocsLayout component # and if there are any other components using it # Search for HideIcon imports and usage rg -l "HideIcon" # Check the DocsLayout component implementation ast-grep --pattern 'import $_ from "./icons/HideIcon"'Length of output: 136
Script:
#!/bin/bash # Let's check how HideIcon is used in DocsLayout.tsx rg "HideIcon" components/layout/DocsLayout.tsx -B 2 -A 2 # Also check for any sidebar toggle related state or functionality ast-grep --pattern 'const [$_] = useState<boolean>' components/layout/DocsLayout.tsxLength of output: 542
Script:
#!/bin/bash # Let's check for the sidebar toggle state and function implementation rg "toggleSidebar|isSidebarVisible" components/layout/DocsLayout.tsx -B 2 -A 2 # Also check if there are any related accessibility attributes or aria labels rg "aria-|role" components/layout/DocsLayout.tsxLength of output: 1119
styles/globals.css (1)
362-364:⚠️ Potential issueImprove color contrast and consider using CSS classes instead of IDs.
- The stark contrast between white and black states might be jarring. Consider using softer colors.
- Using IDs for styling creates high specificity and can lead to maintainability issues.
-button#sidebarToggle.open { - background-color: black; +.sidebar-toggle { + background-color: var(--sidebar-toggle-bg, white); +} + +.sidebar-toggle--open { + background-color: var(--sidebar-toggle-active-bg, #2d2d2d); }Let's verify the color contrast ratios for accessibility:
✅ Verification successful
Let me gather more context about the color-related styles for this button.
Let me check if there are any CSS custom properties (variables) defined in the codebase that we could leverage.
Confirmed: Color contrast and specificity improvements needed
The current implementation has several issues:
- Uses high-specificity ID selector (
button#sidebarToggle)- Has a harsh white-to-black color transition
- Lacks CSS custom properties for maintainable theming
Recommended changes:
- Replace ID with BEM-style classes as suggested
- Use
#2d2d2dinstead of pure black for better visual comfort- Introduce CSS custom properties for colors
- Consider adding hover/focus states for better interactivity
The original review comment's suggestions are valid and would improve both maintainability and user experience.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any other color-related styles for the sidebar toggle # that might affect contrast ratios rg -i "sidebarToggle" --type cssLength of output: 121
Script:
#!/bin/bash # Get the complete style definitions for the sidebar toggle button rg -A 5 "button#sidebarToggle" styles/globals.cssLength of output: 289
Script:
#!/bin/bash # Search for CSS custom properties related to colors or sidebar rg -i "(:root|--.*color|--.*sidebar)" --type cssLength of output: 112
components/layout/DocsLayout.tsx (1)
23-24: LGTM: Icon imports are properly organized.The new icon imports are correctly placed alongside other icon imports.
|
@mohdshubair313 Have you checked this PR's deployment. Seems like the changes you made aren't working. |
asyncapi-bot
requested review from
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
|
@mohdshubair313 it is not working locally |
Yeah because I think the content should be visible in full view to the users If this is the issue so what should I do |
|
@mohdshubair313 i was looking for it. Found some packages like this I will try to submit a PR using simpler approach |
|
Please checkout this @mohdshubair313 fixed it here |
|
@Manancode yeah it works and done all check marks are green but brother this is your PR so It will not happen that your PR gets merged and my PR remains like this and does not merge. |
|
Can I copy this from this PR and make my PR better? Is I have to make a new PR and added what you tell me in your PR |
|
@mohdshubair313 Any update on this PR? |
|
@anshgoyalevil yeah working will submit PR in a day |
|
This PR closes the issue though. Are you working on something more @mohdshubair313 ? Also checkout this |
|
Closing the PR with ref: #3325 (comment) |
…in full view
Description
Related issue(s)
#3325 fixes the not viewing the content in full view but instead I add a toggle button so that when needed users can uses that sidebar
Summary by CodeRabbit
New Features
HideIconandShowIconcomponents for enhanced visual representation.Style