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

a0m0rajab · 2023-07-30T01:34:41Z

Description

This PR adds redirect to profile page like the next:

In the feed page, the mention/tag to username redirects to OpenSauced profile
In the user page, the mention/tag redirects to GitHub Profle

Beside the redirects, it fixes the label/input connection issue in the pages with multiple checkboxes, if the check box has no ID. the default id would be checkbox which will break the UI due to having multiple similar IDs, I fixed this by using the label as ID, to avoid any issue in the future and make the ID as an optional for programmer for easier coding.

What type of PR is this? (check all applicable)

Related Tickets & Documents

Fixes #1447
Fixes #1199
Fixes #1448

Mobile & Desktop Screenshots/Recordings

This link in the profile page is redirecting to GitHub:

To test:

go to a user profile: https://insights.opensauced.pizza/user/a0m0rajab/highlights
click on the mention (tag) under their profile
Check if it's redirecting to the GitHub

This link in the feed page redirects to OpenSauced Profile:

To test

go to the feed page: https://insights.opensauced.pizza/feed
Click on the profile (mention)
Check if it's redirecting to the open sauced user page

The checkbox elements and labels are connected to their related inputs:

To Test you need to do the next:

Go to: https://insights.opensauced.pizza/user/settings
Click on the check boxes labels one and see if their related check box is triggered(display email, collaboration, timezone)

Deploy Test Links:

Added tests?

👍 yes
🙅 no, because they aren't needed
🙋 no, because I need help

Added to documentation?

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

netlify · 2023-07-30T01:34:46Z

❌ Deploy Preview for oss-insights failed.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`9a90431`
🔍 Latest deploy log	https://app.netlify.com/sites/oss-insights/deploys/64c7f2272551bb00081f523d

netlify · 2023-07-30T01:34:47Z

✅ Deploy Preview for design-insights ready!

Name	Link
🔨 Latest commit	`9a90431`
🔍 Latest deploy log	https://app.netlify.com/sites/design-insights/deploys/64c7f22722785300082729c9
😎 Deploy Preview	https://deploy-preview-1449--design-insights.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

a0m0rajab · 2023-07-30T01:38:55Z

components/atoms/Checkbox/checkbox.tsx

- 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();


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)

You should turn this to a function that returns the conditional statements here

Suggested change

id = id ? id : label?.replaceAll(" ", "_").toLowerCase();

const getId = ()=> id ? id : label?.replaceAll(" ", "_").toLowerCase();

Then in the component attributes, set it to id={getId()}

I am going to implement this, but was wondering why we would take this approach?

The variable name needs updating to avoid conflicts with the reserved id keyword from the props.

We could have easily said const newId = id ? id : label?.replaceAll(" ", "_").toLowerCase(); but it makes more sense with a function since we are performing some checks to determine the used id.

a0m0rajab · 2023-07-30T01:39:29Z

components/atoms/Checkbox/checkbox.tsx

- <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) => {


the id had checkbox as default value, I removed it from here.

OgDev-01 · 2023-07-31T12:28:38Z

components/atoms/Checkbox/checkbox.tsx

- 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();


You should turn this to a function that returns the conditional statements here

Suggested change

id = id ? id : label?.replaceAll(" ", "_").toLowerCase();

const getId = ()=> id ? id : label?.replaceAll(" ", "_").toLowerCase();

Then in the component attributes, set it to id={getId()}

components/atoms/Checkbox/checkbox.tsx

babblebey · 2023-07-31T16:08:36Z

components/molecules/ContributorProfileInfo/contributor-profile-info.tsx

@@ -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>


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.

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

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.

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

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.

Re: 3 different issues,

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

open-sauced/ai#163

Re: new PR.
Opened three different PRs.

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

Thank you for giving me information about the context! Was looking at all repos from the same perspective 😄

Co-authored-by: ( Nechiforel David-Samuel ) NsdHSO <[email protected]>

bdougie

Open one PR per issue.

bdougie · 2023-08-01T05:17:43Z

Please do one PR per feature/issue.

a0m0rajab · 2023-08-01T13:09:07Z

Thanks for that, I opened them.

fix: add id to checkbox, add link to profiles

9a7afd7

a0m0rajab commented Jul 30, 2023

View reviewed changes

Typos everywhere :3

5331847

OgDev-01 suggested changes Jul 31, 2023

View reviewed changes

NsdHSO reviewed Jul 31, 2023

View reviewed changes

components/atoms/Checkbox/checkbox.tsx Outdated Show resolved Hide resolved

OgDev-01 added the requested changes label Jul 31, 2023

babblebey reviewed Jul 31, 2023

View reviewed changes

a0m0rajab and others added 3 commits July 31, 2023 19:49

Update components/atoms/Checkbox/checkbox.tsx

42a0442

Co-authored-by: ( Nechiforel David-Samuel ) NsdHSO <[email protected]>

chore: convert id to function

655b2ac

Merge branch 'beta' into fix--checkbox-accesiability,-links-to-pages

de21e67

a0m0rajab requested review from OgDev-01, NsdHSO and babblebey July 31, 2023 16:54

a0m0rajab added needs-approval and removed requested changes labels Jul 31, 2023

OgDev-01 added requested changes and removed needs-approval labels Jul 31, 2023

Merge branch 'beta' into fix--checkbox-accesiability,-links-to-pages

9a90431

bdougie requested changes Aug 1, 2023

View reviewed changes

bdougie closed this Aug 1, 2023

This was referenced Aug 1, 2023

chore: add links to user profile on feeds userCard #1458

Merged

feat: add github link to profile #1459

Merged

a0m0rajab deleted the fix--checkbox-accesiability,-links-to-pages branch August 1, 2023 11:04

a0m0rajab mentioned this pull request Aug 2, 2023

fix: add checkbox id name if not available based on label #1466

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

a0m0rajab commented Jul 30, 2023 •

edited

netlify bot commented Jul 30, 2023 •

edited

netlify bot commented Jul 30, 2023 •

edited

a0m0rajab Jul 30, 2023

OgDev-01 Jul 31, 2023

a0m0rajab Jul 31, 2023

OgDev-01 Jul 31, 2023

a0m0rajab Jul 30, 2023

OgDev-01 Jul 31, 2023

babblebey Jul 31, 2023 •

edited

a0m0rajab Jul 31, 2023

OgDev-01 Jul 31, 2023

a0m0rajab Jul 31, 2023

bdougie Aug 1, 2023

a0m0rajab Aug 1, 2023

bdougie Aug 1, 2023

a0m0rajab Aug 1, 2023

bdougie left a comment

bdougie commented Aug 1, 2023

a0m0rajab commented Aug 1, 2023

	id = id ? id : label?.replaceAll(" ", "_").toLowerCase();
	const getId = ()=> id ? id : label?.replaceAll(" ", "_").toLowerCase();

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

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

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

Conversation

a0m0rajab commented Jul 30, 2023 • edited

Description

What type of PR is this? (check all applicable)

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings

Deploy Test Links:

Added tests?

Added to documentation?

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

netlify bot commented Jul 30, 2023 • edited

❌ Deploy Preview for oss-insights failed.

netlify bot commented Jul 30, 2023 • edited

✅ Deploy Preview for design-insights ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babblebey Jul 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdougie left a comment

Choose a reason for hiding this comment

bdougie commented Aug 1, 2023

a0m0rajab commented Aug 1, 2023

a0m0rajab commented Jul 30, 2023 •

edited

netlify bot commented Jul 30, 2023 •

edited

netlify bot commented Jul 30, 2023 •

edited

babblebey Jul 31, 2023 •

edited