-
Notifications
You must be signed in to change notification settings - Fork 21
feat(PM-3454): Added identity verified badge to profiles page #1416
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
Conversation
| <div className={styles.phonesContent}> | ||
| {phones.length > 0 | ||
| ? phones.map((phone, index: number) => ( | ||
| && phones.map((phone, index: number) => ( |
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.
[design]
The change from a ternary operator to a logical AND operator removes the EmptySection component when there are no phones. Ensure that this behavior is intentional and that the user experience is not negatively impacted by the absence of a message indicating no phone numbers are available.
| @@ -0,0 +1,60 @@ | |||
| @import "@libs/ui/styles/includes"; | |||
|
|
|||
| .badgeContainer { | |||
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.
[💡 maintainability]
Consider using a more descriptive class name than .badgeContainer to improve maintainability and clarity, especially if there are multiple badge types in the application.
| } | ||
| } | ||
|
|
||
| .tooltip { |
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.
[correctness]
The .tooltip class uses a hardcoded color $black-100 for the background. Ensure this variable is defined and consistent with the design system to avoid unexpected styling issues.
| bottom: calc(100% + 8px); | ||
| left: 50%; | ||
| transform: translateX(-50%); | ||
| background-color: $black-100; |
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.
[correctness]
Ensure that $black-100 and $tc-white are defined in the imported styles and are consistent with the application's theme to prevent styling inconsistencies.
| import styles from './IdentityVerifiedBadge.module.scss' | ||
|
|
||
| interface IdentityVerifiedBadgeProps { | ||
| identityVerified?: boolean |
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.
[design]
Consider making identityVerified a required prop instead of optional. This would make the component's behavior more predictable and reduce the need for null checks.
|
|
||
| const IdentityVerifiedBadge: FC<IdentityVerifiedBadgeProps> = (props: IdentityVerifiedBadgeProps) => { | ||
| if (!props.identityVerified) { | ||
| // eslint-disable-next-line unicorn/no-null |
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.
[💡 style]
Returning null is acceptable here, but consider using undefined to align with React's best practices for conditional rendering.
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.
FYI, this is good suggestion and practice to appy going forward @hentrymartin .
| return ( | ||
| <div className={styles.photoWrap}> | ||
| <ProfilePicture member={props.profile} className={styles.profilePhoto} /> | ||
| <IdentityVerifiedBadge identityVerified={props.profile.identityVerified} /> |
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.
[correctness]
Ensure that props.profile.identityVerified is always a boolean. If it can be undefined or null, it might cause the IdentityVerifiedBadge component to behave unexpectedly. Consider adding a default value or type check.
| @@ -0,0 +1,3 @@ | |||
| <svg width="68" height="68" viewBox="0 0 68 68" fill="none" xmlns="http://www.w3.org/2000/svg"> | |||
| <path d="M34 3C51.1208 3 65 16.8792 65 34C65 51.1208 51.1208 65 34 65C16.8792 65 3 51.1208 3 34C3 16.8792 16.8792 3 34 3ZM44.8535 26.6465C44.6583 26.4512 44.3417 26.4512 44.1465 26.6465L30.5 40.293L23.8535 33.6465C23.6583 33.4512 23.3417 33.4512 23.1465 33.6465C22.9512 33.8417 22.9512 34.1583 23.1465 34.3535L30.1465 41.3535L30.2246 41.418C30.4187 41.5461 30.6827 41.5244 30.8535 41.3535L44.8535 27.3535C45.0488 27.1583 45.0488 26.8417 44.8535 26.6465Z" fill="#0AB88A" stroke="white" stroke-width="6"/> | |||
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.
[design]
The stroke-width attribute is set to 6, which is relatively large compared to the SVG's dimensions. Ensure this is intentional, as it might affect the visual appearance of the badge, especially when scaled down.
|
|
||
| const IdentityVerifiedBadge: FC<IdentityVerifiedBadgeProps> = (props: IdentityVerifiedBadgeProps) => { | ||
| if (!props.identityVerified) { | ||
| // eslint-disable-next-line unicorn/no-null |
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.
FYI, this is good suggestion and practice to appy going forward @hentrymartin .
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3454
What's in this PR?
Screenshots