fix: implement feedback #51
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Comments from This PR |
There was a problem hiding this comment.
Pull Request Overview
This PR implements feedback from a previous code review that was merged prematurely. The changes focus on improving React component structure and key prop handling.
- Replace
<p>tags with semantically appropriate<span>elements for inline text - Update React keys to use more descriptive values instead of plain array indices
- Add sorting to examples dropdown for consistent ordering
| const href = `${window.location.origin}/parameters/example/${slug}`; | ||
| return ( | ||
| <DropdownMenuItem | ||
| key={`${slug}-${index}`} |
There was a problem hiding this comment.
Using index in the key after sorting may cause React reconciliation issues. Since slug appears to be unique, consider using just key={slug} instead of including the index.
| key={`${slug}-${index}`} | |
| key={slug} |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
The old code was right. index shouldn't be here
| @@ -98,7 +98,7 @@ export const Editor: FC<EditorProps> = ({ | |||
| <DropdownMenuTrigger className="flex w-fit min-w-[140px] cursor-pointer items-center justify-between rounded-md border bg-surface-primary px-2 py-1.5 text-content-secondary transition-colors hover:text-content-primary data-[state=open]:text-content-primary"> | |||
| <div className="flex items-center justify-center gap-2"> | |||
There was a problem hiding this comment.
We still need to check the generated HTML. If the trigger is producing a button, the resulting markup is going to fail to pass from an HTML validator
#50 (comment)
| @@ -108,7 +108,7 @@ export const Editor: FC<EditorProps> = ({ | |||
| {snippets.map( | |||
| ({ name, label, icon: Icon, snippet }, index) => ( | |||
| <DropdownMenuItem | |||
| key={index} | |||
| key={`${label}-${index}`} | |||
There was a problem hiding this comment.
index should be removed entirely
| @@ -124,28 +124,33 @@ export const Editor: FC<EditorProps> = ({ | |||
| <DropdownMenuTrigger className="flex w-fit min-w-[140px] cursor-pointer items-center justify-between rounded-md border bg-surface-primary px-2 py-1.5 text-content-secondary transition-colors hover:text-content-primary data-[state=open]:text-content-primary"> | |||
| <div className="flex items-center justify-center gap-2"> | |||
There was a problem hiding this comment.
Same feedback here
#50 (comment)
| })} | ||
| {Object.entries(examples) | ||
| .sort() | ||
| .map(([slug, title], index) => { |
There was a problem hiding this comment.
Your code was right before. I'm curious to know why this was changed
| const href = `${window.location.origin}/parameters/example/${slug}`; | ||
| return ( | ||
| <DropdownMenuItem | ||
| key={`${slug}-${index}`} |
There was a problem hiding this comment.
The old code was right. index shouldn't be here
| asChild={true} | ||
| > | ||
| <a href={href} target="_blank" rel="noreferrer"> | ||
| <span className="sr-only"> |
There was a problem hiding this comment.
This screen-reader comment should be placed after the title. Think about how the text would look like if we made everything visible:
(link opens in new tab) Attach GPU (random leading space included)
My trigger finger on mobile was too sensitive and I merged a PR without seeing the comments. Sorry @Parkreiner