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(clerk-js): Align some sections in User Profile with the designs #2814

Conversation

octoper
Copy link
Member

@octoper octoper commented Feb 16, 2024

Description

This PR adds a hover state for all profile section items that include an action menu (eg. Emails, Phone numbers etc.)

Before

CleanShot 2024-02-16 at 15 51 09@2x

After

CleanShot 2024-02-16 at 15 53 57@2x

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Copy link

changeset-bot bot commented Feb 16, 2024

🦋 Changeset detected

Latest commit: cf24ec6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

id='connectedAccounts'
sx={t => ({
borderRadius: t.radii.$lg,
':hover': { backgroundColor: t.colors.$neutralAlpha50 },
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid the duplication here by putting the styles inside ProfileSection.Item ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't move them in ProfileSection.Item as we use it for stuff that don't need the hover state and the border radius but we can create a new component or add a prop to add those styles.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can move them in and add a prop or make a small wrapper as you said. I prefer the wrapper tbh.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only thing I don't like about the wrapper is that it's can be a bit confusing as it would be the same thing, the hover state is like having a variant.

@@ -62,6 +62,7 @@ export const ArrowBlockButton = React.forwardRef<HTMLButtonElement, ArrowBlockBu
justifyContent: 'center',
borderColor: theme.colors.$neutralAlpha100,
alignItems: 'center',
padding: `${theme.space.$1x5} ${theme.space.$3} ${theme.space.$1x5} ${theme.space.$2x5}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary? Isn't it going to make most screens deviate from designs now?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the padding that the designs have.

@@ -128,7 +128,7 @@ const ProfileSectionItem = (props: ProfileSectionItemProps) => {
justifyContent: 'space-between',
width: '100%',
alignItems: 'center',
padding: `${t.space.$1x5} ${t.space.$none} ${t.space.$1x5} ${t.space.$4}`,
padding: `${t.space.$1x5} ${t.space.$3} ${t.space.$1x5} ${t.space.$2x5}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

This change is made to be completely aligned with the designs

@desiprisg
Copy link
Contributor

!preview

@clerk-cookie
Copy link
Collaborator

clerk-cookie commented Feb 16, 2024

Hey @anagstef, your preview is available.

Status Preview Updated (UTC)
🍪 Deployed Visit preview Feb 16, 2024 07:43 PM

@octoper octoper force-pushed the vaggelis/sdk-1362-fix-email-address-highlighted-state-on-userprofile branch 2 times, most recently from ba41574 to 770c57b Compare February 16, 2024 17:00
@octoper octoper force-pushed the vaggelis/sdk-1362-fix-email-address-highlighted-state-on-userprofile branch from 770c57b to 1647a19 Compare February 16, 2024 17:52
@anagstef
Copy link
Member

!preview

@octoper
Copy link
Member Author

octoper commented Feb 16, 2024

!preview

@anagstef
Copy link
Member

!preview

@nikosdouvlis nikosdouvlis merged commit 382ea0e into main Feb 16, 2024
7 checks passed
@nikosdouvlis nikosdouvlis deleted the vaggelis/sdk-1362-fix-email-address-highlighted-state-on-userprofile branch February 16, 2024 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants