Skip to content

Create FAQ Accordion Component #13840

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

saurabhburade
Copy link
Contributor

@saurabhburade saurabhburade commented Sep 10, 2024

Description

Create FaqAccordion Component

Related Issue

Copy link

netlify bot commented Sep 10, 2024

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit bbd9597
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/6717fbc3a5e565000832f4c6
😎 Deploy Preview https://deploy-preview-13840--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
7 paths audited
Performance: 34 (🔴 down 19 from production)
Accessibility: 92 (🔴 down 1 from production)
Best Practices: 86 (🔴 down 12 from production)
SEO: 99 (🟢 up 7 from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@saurabhburade
Copy link
Contributor Author

Currently, using a tailwind accordion code copy with few modifications.

@saurabhburade saurabhburade marked this pull request as ready for review September 10, 2024 10:21
@saurabhburade saurabhburade marked this pull request as draft September 25, 2024 05:52
@saurabhburade saurabhburade marked this pull request as ready for review October 3, 2024 05:37
Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the help here @saurabhburade! Just left some questions, requests, etc =)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accordiOn not accordiAn

Need to fix the file paths here: FaqAccordion/index.tsx
(Same with the story: FaqAccordion/FaqAccordion.stories.tsx)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, this also makes me wonder if we should even be calling this "FaqAccordion", since in the DS it's just called FAQ... @pettinarip Any thoughts here? Simply calling this component/FAQ/index.tsx would work for me, and could just swap "FAQ" for "Accordion" in the exported sub-components in this file.

React.ComponentPropsWithoutRef<typeof AccordionPrimitive.Trigger>
>(({ className, children, ...props }, ref) => (
// p-4 hover:bg-background-highlight
<AccordionPrimitive.Header className="flex w-full">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we didn't go with the existing AccordionTrigger from tailwind/ui/accordion.tsx, instead of rebuilding from the Radix-UI primitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used for grouping styles with MdChevronRight ==> "group-hover:border-primary group-hover:text-primary group-hover:shadow-md group-hover:shadow-primary-low-contrast"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen.Recording.2024-10-22.at.11.14.51.mov

Functioning pretty well, just noting we should handle the width. In the DS we have max-width: 52rem; Ultimately though, the width shouldn't change when open vs closed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can implement max-width: 52rem; or near valuemax-w-4xl ==> 56rem. One thing I notice is, its taking parent width if mentioned and fits it to full width (trigger & content both)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen.Recording.2024-10-23.at.12.38.55.AM.mov

Copy link
Contributor

This issue is stale because it has been open 30 days with no activity.

@github-actions github-actions bot added the Status: Stale This issue is stale because it has been open 30 days with no activity. label Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Stale This issue is stale because it has been open 30 days with no activity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create FAQ component
2 participants