fix(web): Manual chapter - Accordion scroll bugfix #16511
Conversation
RunarVestmann
added
the
deprecated:automerge
WalkthroughThe changes in this pull request focus on enhancing the state management of the accordion component within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/web/screens/Manual/ManualChapter.tsx (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/web/screens/Manual/ManualChapter.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
Datadog ReportBranch report: ✅ 0 Failed, 84 Passed, 0 Skipped, 33.33s Total Time |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16511 +/- ##
==========================================
- Coverage 36.77% 36.76% -0.01%
==========================================
Files 6847 6847
Lines 141872 141884 +12
Branches 40423 40429 +6
==========================================
Hits 52167 52167
- Misses 89705 89717 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/web/screens/Manual/ManualChapter.tsx (1)
189-202: LGTM: Improved state management for collapsing itemsThe logic for collapsing items is well-implemented. It correctly updates both
expandedItemIdsandselectedItemId, maintaining consistency between the two states. This change effectively supports the new behavior of keeping multiple items expanded.A minor optimization could be to use
Array.prototype.filter()instead of splicing:setExpandedItemIds((prev) => prev.filter(id => id !== item.id))This approach is more declarative and potentially easier to read.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/web/screens/Manual/ManualChapter.tsx (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/web/screens/Manual/ManualChapter.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (2)
apps/web/screens/Manual/ManualChapter.tsx (2)
78-80: LGTM: New state variable for managing expanded itemsThe introduction of
expandedItemIdsstate variable is a good approach to track multiple expanded accordion items. This change aligns with the PR objective of allowing multiple items to remain open simultaneously, enhancing the user experience.
Line range hint
1-283: Overall assessment: Changes meet PR objectives with room for minor improvementsThe implemented changes successfully achieve the PR objective of allowing multiple accordion items to remain open simultaneously. The state management has been thoughtfully updated to handle this new behavior, improving the user experience as intended.
While there are no critical issues, consider the following minor optimizations to further enhance the code quality:
- Simplify the
expandedcondition in theAccordionItemprops.- Prevent potential duplicate IDs in the
expandedItemIdsstate.- Use
Array.prototype.filter()for a more declarative approach when removing items fromexpandedItemIds.These suggestions aim to make the code more robust and easier to maintain. Great job on implementing this feature!
* Fix scroll bug in manual chapter accordion * Fix typo --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Manual chapter - Accordion scroll bugfix
What
Why
Screenshots / Gifs
Before
Screen.Recording.2024-10-22.at.14.27.25.mov
After
Screen.Recording.2024-10-22.at.14.28.27.mov
Checklist:
Summary by CodeRabbit