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

Improve phone input UI #8266

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Hitarthsheth07
Copy link
Contributor

@Hitarthsheth07 Hitarthsheth07 commented Nov 1, 2024

[FIX] #8251

Changes made as suggested by @Bonapara.

image
image
image
image

For the The country code should be Tertiary instead of Primary task,
the library "react-phone-number-input" doesn't provide any out of the box functionality to style the country code.

If the feature needs to be implemented here are the possible solution/workarounds:

  1. Finding a more customizable library that allows to change the style of the country code

OR

  1. Implement custom country selection (😰...)

OR

  1. The lib allows a custom input element and a provides a function (onCountryChange) that triggers whenever the country changes (a country can be changed in two ways- 1. When the user deliberately chooses it from dropdown OR 2. Changes the code in the input)

We'll have to get the length of the country code and then style the first X digits in the custom input field...

image
image

Let me know if someone has a better approach.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR improves the phone input UI by adjusting spacing and styling, particularly focusing on the country code picker and input field layout.

  • Modified margin-left in PhonesFieldInput.tsx to add 2 units of spacing between country code picker and input field
  • Adjusted padding in PhoneCountryPickerDropdownButton.tsx for better visual balance (1.25 left, 1 right)
  • Changed StyledInputContainer padding to 0 in DropdownMenuInput.tsx to fix spacing issues
  • Added border-right styling to country picker dropdown button for visual separation
  • Noted limitations of react-phone-number-input library regarding country code color customization

3 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile

padding: 0 ${({ theme }) => theme.spacing(2)}
${({ theme }) => theme.spacing(1)};
padding: 0 ${({ theme }) => theme.spacing(2)};
${({ theme }) => theme.spacing(1)};
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax: Missing template literal - this line won't apply the spacing correctly

@Bonapara
Copy link
Member

Bonapara commented Nov 1, 2024

Thanks @Hitarthsheth07 for your contribution!

Country Code

Let's keep it to color:primary then

Padding

Current

CleanShot 2024-11-01 at 12 51 05@2x

The right padding is too large.

Desired

It should be 4px so that the icon button's top, left, and bottom padding are equal.

CleanShot 2024-11-01 at 12 52 40@2x

Dropdown menu

Current

CleanShot 2024-11-01 at 13 13 54@2x

Width is 50

Desired

Width should be 44

CleanShot 2024-11-01 at 13 15 12@2x

@Hitarthsheth07
Copy link
Contributor Author

Thanks for the feedback @Bonapara

Here are the changes:

Dropdown menu

I've added the left padding 8px and right padding 4x.
Icons and gap now have proper attributes.

The overall width is still 44.8px because it also takes into account the border.
image

While in figma we are using gap to show border, so the overall width of the left container remains same.

https://www.loom.com/share/11ddb9ba2402479bbdb6b306e30215cc?sid=5b0da58a-42f3-4bf8-8fa8-21bf870c72e2

This could be solved by changing the bg color of the parent and children div and then adding gap to them, similar approach to what you're doing in figma.
Let me know if want to implement this.

Right Add icon

As suggested padding on all the sides is even now.
image

@Bonapara
Copy link
Member

Bonapara commented Nov 4, 2024

Hi @Hitarthsheth07, regarding the padding, I'm referring to the gap between the icon and input edge:

3f9df329bb697621da0eb49e6238bf34cbb4d7f06faaec30c9e595585dc0c59f

Also your PR seems to cause the picker to freeze when I click on the three dots menu:

CleanShot.2024-11-04.at.10.38.52.mp4

@Hitarthsheth07
Copy link
Contributor Author

Hi @Hitarthsheth07, regarding the padding, I'm referring to the gap between the icon and input edge:

3f9df329bb697621da0eb49e6238bf34cbb4d7f06faaec30c9e595585dc0c59f

Also your PR seems to cause the picker to freeze when I click on the three dots menu:

CleanShot.2024-11-04.at.10.38.52.mp4

Ahh got it!

@Hitarthsheth07
Copy link
Contributor Author

@Bonapara btw, can you check if there are any error in the browser console when you click on the three dots.

I've only made changes to CSS styles and it shouldn't affect anything else.

I do get error when clicked on three dots in my local setup and the app freezes without even changing any code.

@Bonapara
Copy link
Member

Bonapara commented Nov 4, 2024

@Hitarthsheth07 The bug was actually caused by something else and has been fixed on the main branch. I updated your branch to the main, and the bug is gone. Let's forget about this issue and focus on fixing the icon padding!

@Hitarthsheth07
Copy link
Contributor Author

@Hitarthsheth07 The bug was actually caused by something else and has been fixed on the main branch. I updated your branch to the main, and the bug is gone. Let's forget about this issue and focus on fixing the icon padding!

On it, will push the commit in an hour or two.

@Hitarthsheth07
Copy link
Contributor Author

image

@Bonapara

@@ -95,7 +103,9 @@ export const PhoneCountryPickerDropdownButton = ({
<StyledDropdownButtonContainer isUnfolded={isDropdownOpen}>
<StyledIconContainer>
{selectedCountry ? <selectedCountry.Flag /> : <IconWorld />}
<IconChevronDown size={theme.icon.size.sm} />
<StyledCheveronIconContainer>
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add a styled container?

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 chevron down icon is not sized as per the figma file, the only way to change the size is by adding a container

@Bonapara
Copy link
Member

Bonapara commented Nov 5, 2024

image

It seems I have a 1px gap with the cell. Is this the case for you as well? It should be on top of the cell border.

@Hitarthsheth07
Copy link
Contributor Author

image

@Bonapara the gap between cell and dialog box is already there in this version.
Should I push a commit to change it?

@Bonapara
Copy link
Member

Bonapara commented Nov 5, 2024

You're right! I think we will fix it in the release quick fixes, so no need thanks!

@Hitarthsheth07
Copy link
Contributor Author

You're right! I think we will fix it in the release quick fixes, so no need thanks!

Sounds good! Can this be merged and closed

Thanks.

@Bonapara
Copy link
Member

Bonapara commented Nov 5, 2024

@lucasbordeau if you can do the review - lgtm from a product perspective

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants