Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: add id to checkbox, add link to profiles #1449

Closed
wants to merge 6 commits into from
Closed

fix: add id to checkbox, add link to profiles #1449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9a7afd7
fix: add id to checkbox, add link to profiles
a0m0rajab Jul 30, 2023
5331847
Typos everywhere :3
a0m0rajab Jul 30, 2023
42a0442
Update components/atoms/Checkbox/checkbox.tsx
a0m0rajab Jul 31, 2023
655b2ac
chore: convert id to function
a0m0rajab Jul 31, 2023
de21e67
Merge branch 'beta' into fix--checkbox-accesiability,-links-to-pages
a0m0rajab Jul 31, 2023
9a90431
Merge branch 'beta' into fix--checkbox-accesiability,-links-to-pages
a0m0rajab Jul 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 27 additions & 24 deletions components/atoms/Checkbox/checkbox.tsx
View file
Open in desktop
Expand Up @@ -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) => {
Copy link
Contributor Author

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.

const getId = () => id ? id : label?.replaceAll(" ", "_").toLowerCase();
return (
<div className="flex items-center">
<CheckboxPrimitive.Root
ref={ref}
className={clsx(
"peer h-4 w-4 shrink-0 rounded 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={getId()}
>
{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={getId()}
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;

Expand Down
4 changes: 2 additions & 2 deletions components/atoms/UserCard/user-card.tsx
View file
Open in desktop
Expand Up @@ -32,9 +32,9 @@ const UserCard = ({ username, name, meta, loading }: UserCardProps) => {
src={avatarUrl}
alt={`${username}'s avatar image`}
/>
<div>
<div className="text-center">
<h3 className="text-base text-center">{name}</h3>
<p className="text-center text-light-slate-9">{`@${username}`}</p>
<a className="text-center text-light-slate-9" href={`/user/${username}`}>{`@${username}`}</a>
</div>
</div>
<div className="flex items-center gap-5 text-base text-center ">
Expand Down
2 changes: 1 addition & 1 deletion components/molecules/ContributorProfileInfo/contributor-profile-info.tsx
View file
Open in desktop
Expand Up @@ -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>
Copy link
Contributor

@babblebey babblebey Jul 31, 2023 •
edited

Choose a reason for hiding this comment

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

Suggested change
<a className="text-light-slate-11 text-sm" href={`https://github.com/${githubName}`}>{`@${githubName}`}</a>
<a className="text-light-slate-11 text-sm" href={`/user/${username}`}>{`@${githubName}`}</a>

I don't think the @[username] UI element speaks/communicate well enough of the location it takes a user to on click action, I think its best for User Experience to either have this just link back to the same profile on open-sauced/insights or not link at all.

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 Icon/button somewhere on the UserCard, best in the Socials section below the About, this because the @[username] link alone doesn't tell me by itself its a link to the contributors' github profile.

This compared to the current set-up better informs the users of what they're clicking on and where its should take them to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
https://deploy-preview-1449--oss-insights.netlify.app/user/a0m0rajab/highlights

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bdougie already agreed on that in the previous issue though,

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re: 3 different issues,

I thought this is an accepted pattern since I saw it before in the AI repo
image

open-sauced/ai#163

Re: new PR.
Opened three different PRs.

Copy link
Member

Choose a reason for hiding this comment

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

I thought this is an accepted pattern since I saw it before in the AI repo

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 && (
<>
Expand Down