feature(website): sections for new landing page #791
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Visit the preview URL for this PR (updated for commit 581f629): https://si-admin-staging--pr791-pranav-new-landing-p-xevh9bsb.web.app (expires Tue, 30 Apr 2024 02:11:55 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
|
@mkue |
| {questions.map((question, index) => ( | ||
| <AccordionItem key={index} value={`item-${index}`}> | ||
| <AccordionTrigger> | ||
| <Typography size="lg" className="text-left"> |
There was a problem hiding this comment.
Is the "text-left" required? Seems to me that this should be the default and part of the AccordionTrigger.
| <hr /> | ||
| <Accordion type="single" collapsible className="mb-10 w-full"> | ||
| {questions.map((question, index) => ( | ||
| <AccordionItem key={index} value={`item-${index}`}> |
There was a problem hiding this comment.
Using the index for the key is apparently bad practice (https://react.dev/learn/tutorial-tic-tac-toe#picking-a-key). I know, I did this everywhere🙈 I just learned recently that it should be avoided...
|
looks good to me🙌🏼 |
| @@ -92,17 +92,22 @@ | |||
| "section-7": { | |||
| "title-1": "Social Income combines three approaches to change." | |||
| }, | |||
| "section-8": { | |||
| "section-x": { | |||
|
Works |
|
@mkue Can you merge this PR, I'll quickly take up the next sections... |
There was a problem hiding this comment.
@DarkMenacer, I think you should be able to merge this yourself, as @mkue has already reviewed it and one approval should make it mergeable for you. Please let me know if this option is somehow not available to you.
|
Okay, got it |
|
It says this |
|
@DarkMenacer ah I know why, when things change in the UI folder @renestalder is asked an additional review. @renestalder could you click "approved" (no review required). Thanks |
There was a problem hiding this comment.
@ssandino @DarkMenacer You might wanna remove me from all code owner assignments and code review tasks. I'm so out of the loop, looking at the code I feel like you are doing anything in the code style we have in that project, but I would anyway disagree with many things because that's not the way I'd consider it best practice. My reviews only lead to many comments that are not particular helpful in this code base. I've become a very picky code reviewer as I look very closely at things like accessibility and performance. And I lead teams with focus of sustainability of code bases, that remove Tailwind and unnecessary third-party React dependencies. Nothing of that is particilar useful for Social Income at the moment.
| )); | ||
| AccordionContent.displayName = AccordionPrimitive.Content.displayName; | ||
|
|
||
| export { Accordion, AccordionContent, AccordionItem, AccordionTrigger }; |
There was a problem hiding this comment.
Add the export directly to the corresponding functions/const declarations. There is no benefit of collecting them at the end of the file.
There was a problem hiding this comment.
@DarkMenacer thank you for considering this for a future PR.
There was a problem hiding this comment.
This is definitely fine as the code directly comes from https://ui.shadcn.com/docs/components/accordion and does not need to be changed apart from the tailwind styling...
| @@ -0,0 +1,56 @@ | |||
| 'use client'; | |||
|
|
|||
| import * as AccordionPrimitive from '@radix-ui/react-accordion'; | |||
There was a problem hiding this comment.
Consider using the native <details> element with additional group closing handlers instead of adding a dependency on top that is probably already obsolete.
We benefit from using what is already there in HTML.
There was a problem hiding this comment.
@DarkMenacer thank you for considering this for a future PR.
There was a problem hiding this comment.
I don't like that this is just called accordion2. That is not a semantically useful name and I smell that there is probably already an accordion that could have been extended instead of adding another component with a not so descriptive name.
There was a problem hiding this comment.
@renestalder yes, renaming to the standard name accordion is planned following the migration of the new design.
As discussed in #791 with Rene: he requested to be replaced as code owner. I added instead the three regular code owners until further notice. Additionally, I have added our private GitHub dev account (account used for this PR) as a code owner for the website and shared folder, enabling quicker fixes, such as typos and similar issues.
As discussed in #791 with Rene: he requested to be replaced as code owner. I added instead the three regular code owners until further notice. Additionally, I have added our private GitHub dev account (account used for this PR) as a code owner for the website and shared folder, enabling quicker fixes, such as typos and similar issues.
No description provided.