-
Notifications
You must be signed in to change notification settings - Fork 3
fix: implement feedback #51
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
Changes from 2 commits
8d02b55
616cf91
6673829
fa6c04b
e657681
ed9f884
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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"> | ||||||
<ZapIcon width={18} height={18} /> | ||||||
<p className="text-xs">Snippets</p> | ||||||
<span className="text-xs">Snippets</span> | ||||||
</div> | ||||||
<PlusIcon width={18} height={18} /> | ||||||
</DropdownMenuTrigger> | ||||||
|
@@ -108,7 +108,7 @@ export const Editor: FC<EditorProps> = ({ | |||||
{snippets.map( | ||||||
({ name, label, icon: Icon, snippet }, index) => ( | ||||||
<DropdownMenuItem | ||||||
key={index} | ||||||
key={`${label}-${index}`} | ||||||
brettkolodny marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
onClick={() => onAddSnippet(name, snippet)} | ||||||
> | ||||||
<Icon size={24} /> | ||||||
|
@@ -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"> | ||||||
brettkolodny marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
<NotebookPenIcon width={18} height={18} /> | ||||||
<p className="text-xs">Examples</p> | ||||||
<span className="text-xs">Examples</span> | ||||||
</div> | ||||||
<ChevronDownIcon width={18} height={18} /> | ||||||
</DropdownMenuTrigger> | ||||||
|
||||||
<DropdownMenuPortal> | ||||||
<DropdownMenuContent> | ||||||
{Object.entries(examples).map(([slug, title]) => { | ||||||
const href = `${window.location.origin}/parameters/example/${slug}`; | ||||||
return ( | ||||||
<DropdownMenuItem key={slug} asChild={true}> | ||||||
<a href={href} target="_blank" rel="noreferrer"> | ||||||
<span className="sr-only"> | ||||||
{" "} | ||||||
(link opens in new tab) | ||||||
</span> | ||||||
<ExternalLinkIcon /> | ||||||
{title} | ||||||
</a> | ||||||
</DropdownMenuItem> | ||||||
); | ||||||
})} | ||||||
{Object.entries(examples) | ||||||
Parkreiner marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
.sort() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One last change: because we didn't give a callback, |
||||||
.map(([slug, title], index) => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your code was right before. I'm curious to know why this was changed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed it to add |
||||||
const href = `${window.location.origin}/parameters/example/${slug}`; | ||||||
return ( | ||||||
<DropdownMenuItem | ||||||
key={`${slug}-${index}`} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using index in the key after sorting may cause React reconciliation issues. Since slug appears to be unique, consider using just
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old code was right. |
||||||
asChild={true} | ||||||
> | ||||||
<a href={href} target="_blank" rel="noreferrer"> | ||||||
<span className="sr-only"> | ||||||
brettkolodny marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
{" "} | ||||||
(link opens in new tab) | ||||||
</span> | ||||||
<ExternalLinkIcon /> | ||||||
{title} | ||||||
</a> | ||||||
</DropdownMenuItem> | ||||||
); | ||||||
})} | ||||||
</DropdownMenuContent> | ||||||
</DropdownMenuPortal> | ||||||
</DropdownMenu> | ||||||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Triggers do generate a button, so I've changed the
div
s tospan
s