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.

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: Prevent OSCR score from popping out of dev card with long usernames #4010

Merged
merged 5 commits into from
Sep 19, 2024

Conversation

Mandeep56Singh
Copy link
Contributor

Description

This PR addresses the bug where a long username in a developer card causes the OSCR score to overflow and pop out of the card's boundary while also address related to it.

  • resolve Hydaration error in Devcard component by using useEffect and useState for props.isLoading.
  • fix styling in front view of card by limiting its width,text-center and wrapping
  • fix styling in back view of card by truncate tailwind utility
  • increase font size of oscr limit from 0.5 rem to 0.7rem to make it visible

closes #3981

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings

Front view at max username length

Screenshot (438)

Back view at lengthy username

Screenshot (435)

username in next line

Screenshot (437)

Steps to QA

Tier (staff will fill in)

  • Tier 1
  • Tier 2
  • Tier 3
  • Tier 4

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

Copy link

netlify bot commented Aug 22, 2024

Deploy Preview for oss-insights ready!

Name Link
🔨 Latest commit c6d5128
🔍 Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/66ec21d57005be00089761ba
😎 Deploy Preview https://deploy-preview-4010--oss-insights.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

netlify bot commented Aug 22, 2024

Deploy Preview for design-insights ready!

Name Link
🔨 Latest commit c6d5128
🔍 Latest deploy log https://app.netlify.com/sites/design-insights/deploys/66ec21d5d81078000870ab97
😎 Deploy Preview https://deploy-preview-4010--design-insights.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@brandonroberts
Copy link
Contributor

@Mandeep56Singh is this ready for review?

@nickytonline
Copy link
Member

I was going to suggest not showing the OSCR on the back of the card since it's already on the front and leave as much real estate for the username. Thoughts @brandonroberts @zeucapua @isabensusan?

@Mandeep56Singh
Copy link
Contributor Author

@brandonroberts actually I had hydration error while solving this issue, To my understanding It was due to props.isLoading.
However I have fixed it. I would require your assistance on this.

@Mandeep56Singh Mandeep56Singh marked this pull request as ready for review August 26, 2024 18:03
@brandonroberts
Copy link
Contributor

I was going to suggest not showing the OSCR on the back of the card since it's already on the front and leave as much real estate for the username. Thoughts @brandonroberts @zeucapua @isabensusan?

I agree

@BekahHW
Copy link
Member

BekahHW commented Aug 27, 2024

@nickytonline we have it on the back of the card like that to provide more context - it's out of 300. I think if we remove it here we need to have it referenced somewhere else on the card.

@nickytonline
Copy link
Member

@nickytonline we have it on the back of the card like that to provide more context - it's out of 300. I think if we remove it here we need to have it referenced somewhere else on the card.

Maybe we move it somewhere else on the card on the back. It looks nice to see a full username on the back instead of it being cut off.

@Mandeep56Singh
Copy link
Contributor Author

@BekahHW we can it in front, we just need to add a line displaying
there are various ways for it

we can display the out of message under score, but when we have long username ( 2 lines ) then it will look ugly

Screenshot (442)

displaying it with OSCR score

Screenshot (440)

I found this good, since we don't really need "score" in text as OSCR itself is rating

Screenshot (443)

@Mandeep56Singh
Copy link
Contributor Author

@nickytonline full username on the back can take a lot of space as longest username can be of 39 characters, it would require two lines. I think adding more statistics on the back side, so user can know more about the person. We can remove the username, profile pic and oscr score from back.

we have different option for it.

added new statistics and remove header of back view

Screenshot (444)

this looks good

Screenshot (445)

what are your thoughts ?

@BekahHW
Copy link
Member

BekahHW commented Aug 28, 2024

I would prefer the original fix here. It solves the initial problem without a redesign. Maybe @isabensusan has thoughts.

@isabensusan
Copy link
Member

@Mandeep56Singh I also prefer the original original fix! Let's truncate longer user names

@Mandeep56Singh
Copy link
Contributor Author

@isabensusan ok, the front view also have overflowing problem, the longest github username can be 39 characters long, so it will be two lines, I thought of centering the username.
what you are your thoughts ?
thanks.

Screenshot (437)
Screenshot (438)

@isabensusan
Copy link
Member

hey @Mandeep56Singh ! Yes, centering looks good :)

@Mandeep56Singh
Copy link
Contributor Author

hey @Mandeep56Singh ! Yes, centering looks good :)

Thanks a lot, @isabensusan! I really appreciate the feedback.
Looking forward to making more contributions to OpenSauced!"

@isabensusan isabensusan self-requested a review September 17, 2024 19:43
Copy link
Member

@isabensusan isabensusan left a comment

Choose a reason for hiding this comment

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

Design wise, looks good to me!

@@ -38,6 +39,13 @@ export default function DevCard(props: DevCardProps) {
}
}, [props.isInteractive, props.isFlipped]);

// used to solve hydartion error
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a hook for this already named useHasMounted that can be used instead.

import { useHasMounted } from "lib/hooks/useHasMounted";

const hasMounted = useHasMounted();

Copy link
Member

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

Works great @Mandeep56Singh! One small change request and this is good to go.

CleanShot 2024-09-18 at 08 41 44

components/molecules/DevCard/dev-card.tsx Outdated Show resolved Hide resolved
Copy link
Member

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes @Mandeep56Singh! 🚢

@nickytonline nickytonline dismissed brandonroberts’s stale review September 18, 2024 15:52

Brandon is off today, and his change requests have been addressed.

@nickytonline
Copy link
Member

When you have a chance @zeucapua, we need your approval. Thanks!

auto-merge was automatically disabled September 19, 2024 13:24

Pull request was closed

@nickytonline nickytonline reopened this Sep 19, 2024
@brandonroberts brandonroberts merged commit b92f477 into open-sauced:beta Sep 19, 2024
15 checks passed
open-sauced bot pushed a commit that referenced this pull request Sep 19, 2024
## [2.63.0-beta.3](v2.63.0-beta.2...v2.63.0-beta.3) (2024-09-19)

### 🐛 Bug Fixes

* Prevent OSCR score from popping out of dev card with long usernames ([#4010](#4010)) ([b92f477](b92f477))
open-sauced bot pushed a commit that referenced this pull request Sep 19, 2024
## [2.63.0](v2.62.0...v2.63.0) (2024-09-19)

### 🍕 Features

* updated `/explore` page ([#4053](#4053)) ([37ee0f5](37ee0f5))

### 🐛 Bug Fixes

* Prevent OSCR score from popping out of dev card with long usernames ([#4010](#4010)) ([b92f477](b92f477))
* redirect all explore requests to the new explore page ([#4112](#4112)) ([182f091](182f091))
* set workspaces for `/explore` "Discover Workspaces" section ([#4106](#4106)) ([41c7248](41c7248))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Dev Card with long username cause OSCR to pop out of card
5 participants