feat(web): FAQ List and Accordion - Set single expand to false #15984
Conversation
WalkthroughThe changes involve the Changes
Possibly related PRs
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
|
Datadog ReportBranch report: ✅ 0 Failed, 84 Passed, 0 Skipped, 29.96s Total Time |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15984 +/- ##
==========================================
+ Coverage 36.70% 36.78% +0.08%
==========================================
Files 6776 6722 -54
Lines 139608 137613 -1995
Branches 39691 39167 -524
==========================================
- Hits 51243 50623 -620
+ Misses 88365 86990 -1375
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 307 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/island-ui/contentful/src/lib/FaqList/FaqList.tsx (2)
Line range hint
1-53: Enhance TypeScript usage for better type safety.The component makes good use of TypeScript, but there's room for improvement:
Consider using more specific types for the
answerproperty in thequestionsarray. Currently, it's typed asSliceType[]with an optional chaining operator, but it's always passed to therichTextfunction. You could create a custom type that ensuresansweris always an array ofSliceType.The
localeprop has a default value of 'is', but theLocaletype might include other options. Consider creating a union type for supported locales to make it more explicit.Here's a suggested improvement for the types:
import { Locale } from '@island.is/shared/types' type SupportedLocale = 'is' | 'en' // Add other supported locales as needed type Question = { id: string question: string answer: SliceType[] // Remove optional chaining as it's always used } export interface FaqListProps { title: string questions: Question[] showTitle?: boolean locale?: SupportedLocale }This change will provide better type safety and make the component's API more explicit.
Line range hint
1-53: Consider performance optimization for large FAQ lists.The current implementation renders all FAQ items at once, which could lead to performance issues for very large lists. Consider implementing lazy loading or virtualization for better performance with large datasets.
You could use a library like
react-windoworreact-virtualizedto implement virtualization. Here's a high-level example of how you might modify the component to usereact-window:import { FixedSizeList as List } from 'react-window'; // ... (rest of the imports) const FaqList: FC<React.PropsWithChildren<FaqListProps>> = ({ title, questions, showTitle = true, locale = 'is', }) => { const renderRow = ({ index, style }) => { const { id, question, answer } = questions[index]; return ( <div style={style}> <AccordionItem key={id} id={`faq_${id}`} label={question} labelUse="h2" > {richText(answer, undefined, locale)} </AccordionItem> </div> ); }; return ( <Stack space={6}> {title && showTitle !== false && ( <Text variant="h2" as="h2"> <span data-sidebar-link={slugify(title)}>{title}</span> </Text> )} <Accordion singleExpand={false}> <List height={400} // Adjust based on your needs itemCount={questions.length} itemSize={100} // Adjust based on average item size width="100%" > {renderRow} </List> </Accordion> </Stack> ); };This approach would significantly improve performance for large FAQ lists by only rendering the visible items.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/web/components/Organization/Slice/AccordionSlice/AccordionSlice.tsx (1 hunks)
- libs/island-ui/contentful/src/lib/FaqList/FaqList.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/web/components/Organization/Slice/AccordionSlice/AccordionSlice.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."
libs/island-ui/contentful/src/lib/FaqList/FaqList.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (3)
libs/island-ui/contentful/src/lib/FaqList/FaqList.tsx (2)
36-36: LGTM:singleExpandprop change aligns with PR objectives.The change to set
singleExpand={false}on the Accordion component correctly implements the desired behavior of allowing multiple FAQ items to remain open simultaneously. This aligns with the PR objectives to enhance user experience by preventing previously opened items from closing when a new item is opened.
Line range hint
1-53: Verify reusability across different NextJS apps.The component seems to be designed for reusability, as it's located in the
libsdirectory. However, to ensure it adheres to best practices for reusability across different NextJS apps, consider the following:
- The component uses
@island.is/island-ui/corefor UI elements. Ensure this dependency is available and consistent across all NextJS apps that will use this component.- The
Localetype is imported from@island.is/shared/types. Verify that this shared type is accessible in all NextJS apps where this component will be used.To confirm the availability and consistency of these dependencies across different NextJS apps, run the following script:
This will help ensure that the component remains reusable across different NextJS apps.
apps/web/components/Organization/Slice/AccordionSlice/AccordionSlice.tsx (1)
63-63: LGTM: Change aligns with PR objectives and improves user experience.The modification to set
singleExpand={false}on theAccordioncomponent successfully implements the desired functionality described in the PR objectives. This change allows multiple FAQ items to remain open simultaneously, enhancing the user experience by preventing the closure of previously opened items when a new one is expanded.The implementation adheres to NextJS best practices and makes good use of TypeScript for type safety. The change is straightforward and doesn't introduce any apparent performance implications.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
FAQ List and Accordion - Set single expand to false
What
Why
Screenshots / Gifs
Before
Screen.Recording.2024-09-12.at.16.41.33.mov
After
Screen.Recording.2024-09-12.at.16.42.27.mov
Summary by CodeRabbit
singleExpandproperty set tofalse.