-
-
Notifications
You must be signed in to change notification settings - Fork 201
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
fix: add id to checkbox, add link to profiles #1449
Changes from 2 commits
9a7afd7
5331847
42a0442
655b2ac
de21e67
9a90431
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 | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -9,31 +9,34 @@ interface CheckboxProps extends React.ComponentPropsWithoutRef<typeof CheckboxPr | |||||||
} | ||||||||
|
||||||||
const Checkbox = React.forwardRef<React.ElementRef<typeof CheckboxPrimitive.Root>, CheckboxProps>( | ||||||||
({ className, label, id = "checkbox", ...props }, ref) => ( | ||||||||
<div className="flex items-center"> | ||||||||
<CheckboxPrimitive.Root | ||||||||
ref={ref} | ||||||||
className={clsx( | ||||||||
"peer h-4 w-4 shrink-0 rounded-[4px] cursor-pointer bg-white border border-light-slate-8 hover:border-orange-500 focus:outline-none disabled:cursor-not-allowed disabled:opacity-50 data-[state=checked]:border-orange-500 data-[state=checked]:bg-orange-500", | ||||||||
className | ||||||||
)} | ||||||||
{...props} | ||||||||
id={id} | ||||||||
> | ||||||||
<CheckboxPrimitive.Indicator className={clsx("flex items-center justify-center text-white")}> | ||||||||
<FiCheck className="w-full h-full" /> | ||||||||
</CheckboxPrimitive.Indicator> | ||||||||
</CheckboxPrimitive.Root> | ||||||||
{label && ( | ||||||||
<label | ||||||||
htmlFor={id} | ||||||||
className="ml-3 text-sm font-medium leading-none cursor-pointer text-light-slate-12 peer-disabled:cursor-not-allowed peer-disabled:opacity-70" | ||||||||
({ className, label, id, ...props }, ref) => { | ||||||||
id = id ? id : label?.replaceAll(" ", "_").toLowerCase(); | ||||||||
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. This line is the line that I changed here, plus the ID call in the function, yet git did not detect it only (not sure why) 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. You should turn this to a function that returns the conditional statements here
Suggested change
Then in the component attributes, set it to 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 am going to implement this, but was wondering why we would take this approach? 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 variable name needs updating to avoid conflicts with the reserved We could have easily said |
||||||||
return ( | ||||||||
<div className="flex items-center"> | ||||||||
<CheckboxPrimitive.Root | ||||||||
ref={ref} | ||||||||
className={clsx( | ||||||||
"peer h-4 w-4 shrink-0 rounded-[4px] cursor-pointer bg-white border border-light-slate-8 hover:border-orange-500 focus:outline-none disabled:cursor-not-allowed disabled:opacity-50 data-[state=checked]:border-orange-500 data-[state=checked]:bg-orange-500", | ||||||||
a0m0rajab marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
className | ||||||||
)} | ||||||||
{...props} | ||||||||
id={id} | ||||||||
> | ||||||||
{label} | ||||||||
</label> | ||||||||
)} | ||||||||
</div> | ||||||||
) | ||||||||
<CheckboxPrimitive.Indicator className={clsx("flex items-center justify-center text-white")}> | ||||||||
<FiCheck className="w-full h-full" /> | ||||||||
</CheckboxPrimitive.Indicator> | ||||||||
</CheckboxPrimitive.Root> | ||||||||
{label && ( | ||||||||
<label | ||||||||
htmlFor={id} | ||||||||
className="ml-3 text-sm font-medium leading-none cursor-pointer text-light-slate-12 peer-disabled:cursor-not-allowed peer-disabled:opacity-70" | ||||||||
> | ||||||||
{label} | ||||||||
</label> | ||||||||
)} | ||||||||
</div> | ||||||||
); | ||||||||
} | ||||||||
); | ||||||||
Checkbox.displayName = CheckboxPrimitive.Root.displayName; | ||||||||
|
||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -54,7 +54,7 @@ const ContributorProfileInfo = ({ | |||||
{isConnected && <Badge text="beta" />} | ||||||
</div> | ||||||
<div className="flex items-center text-sm gap-3"> | ||||||
<span className="text-light-slate-11 text-sm">{`@${githubName}`}</span> | ||||||
<a className="text-light-slate-11 text-sm" href={`https://github.com/${githubName}`}>{`@${githubName}`}</a> | ||||||
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.
Suggested change
I don't think the I understand your thought of having a link to the Contributor' GitHub profile from insights Yes, but I would suggest we create a link to the profile explicitly with an This compared to the current set-up better informs the users of what they're clicking on and where its should take them to. 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. If we have the user back to their profile, then it would not make sense to add a link here. 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 agree with @babblebey on this. We should ignore making a link to Github from the user's profile for now. we can revisit this in the future and maybe add some design help from @isabensusan. 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. @bdougie already agreed on that in the previous issue though, 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 agreed on the PR, code review is part of the process. I also stand by my previous feedback in the issue. It is not clear what the goal is in this PR. The title seems like this PR is two features in on PR. This PR links 3 different issues. I don't know where to begin on the feedback or review. I'd suggest closing this PR and opening separate PRs. 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. 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.
This is a repo used by close to 4k users. We can close multiple issues when it makes sense, but your changes were completely different and your title and descriptions were not cohesive. There are a lot more π here than the AI repo. Please consider the context 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. Thank you for giving me information about the context! Was looking at all repos from the same perspective π |
||||||
|
||||||
{isConnected && ( | ||||||
<> | ||||||
|
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.
the
id
had checkbox as default value, I removed it from here.