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(NcInputField): Bring input field height to 44px and fixes its focus feedback #4718

Merged
merged 5 commits into from
Nov 7, 2023

Conversation

marcoambrosini
Copy link
Contributor

@marcoambrosini marcoambrosini commented Oct 31, 2023

* bring input field height to 44px and fixes its focus feedback

Screen.Recording.2023-10-31.at.13.00.37.mov

* bring select height down to 44px and improves a bit the looks of the select chips

Screenshot 2023-11-07 at 10 21 13

Signed-off-by: Marco <marcoambrosini@icloud.com>
Signed-off-by: Marco <marcoambrosini@icloud.com>
@marcoambrosini marcoambrosini self-assigned this Nov 1, 2023
@marcoambrosini marcoambrosini added enhancement New feature or request 3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Nov 1, 2023
Signed-off-by: Marco <marcoambrosini@icloud.com>

Fix text inputs focus feedback

Signed-off-by: Marco <marcoambrosini@icloud.com>
Copy link

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Nice! Definitely an improvement. Suggestion: for the tags itself, we use a background color instead of more borders:
(also looks a bit off for single select)
image

When we show buttons and inputs next to each other, the alignment still seems to be an issue. when the field is focused, technically the button and input field are center aligned, but when unfocused it looks like it is a couple of pixels too low. What do you think?

Recording.2023-11-01.141616.mp4

Also, the bottom of the label for the input field gets cut off on focus for some resolutions, and the close button on the right is not centered (dunno if it is related to this PR)

image

@marcoambrosini
Copy link
Contributor Author

Suggestion: for the tags itself, we use a background color instead of more borders

@nimishavijay I tried that but it results in not enough contrast with the user avatars in user selects.

@JuliaKirschenheuter
Copy link
Contributor

Hi @marcoambrosini,

unfortunately the issue with truncation got worse ;(

Screenshot from 2023-11-02 15-43-53

Could you please use ARCToolkit tool with "Check Text Spacing" function to check the results?

Screenshot from 2023-11-02 15-46-14

@nimishavijay
Copy link

I tried that but it results in not enough contrast with the user avatars in user selects.

Do we need a lot of contrast there though? I imagine it would similar to the user bubble when you are @ mentioning a user on Talk. The x button on each user, which I am assuming is the only clickable element, would have enough contrast against the background anyway, what do you think?
image

@marcoambrosini
Copy link
Contributor Author

marcoambrosini commented Nov 6, 2023

@nimishavijay this is very bad in talk too. I guess we need to rethink the avatars again. Without even talking about accessibility, this is not acceptable from a visual design standpoint. (The colors have almost exactly the same luminance and it looks like a bug)

Screenshot 2023-11-06 at 18 28 18

I'll remove the border here and raise an issue with the avatars

Signed-off-by: Marco <marcoambrosini@icloud.com>
Signed-off-by: Marco <marcoambrosini@icloud.com>
@marcoambrosini
Copy link
Contributor Author

@JuliaKirschenheuter @nimishavijay could you please review again?

@marcoambrosini marcoambrosini marked this pull request as ready for review November 7, 2023 05:06
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

The trailing button is not centered anymore:
image

Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

Trancation is gone ;)
Screenshot from 2023-11-07 11-53-58

But a placeholder is not centered
Screenshot from 2023-11-07 11-54-09

I would say we can live with it, all design improvements can be done later

Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Agree with @JuliaKirschenheuter :) Let’s merge it as is now and @marcoambrosini go for an improvement pull request.

@susnux susnux changed the title Enh/equalize input heights fix(NcInputField): Bring input field height to 44px and fixes its focus feedback Nov 7, 2023
@susnux susnux added bug Something isn't working and removed enhancement New feature or request labels Nov 7, 2023
@susnux susnux merged commit fce4ae4 into master Nov 7, 2023
16 checks passed
@susnux susnux deleted the enh/equalize-input-heights branch November 7, 2023 12:46
@susnux susnux mentioned this pull request Nov 7, 2023
2 tasks
@marcoambrosini
Copy link
Contributor Author

@JuliaKirschenheuter are you sure that you don't have any CSS interfering with the component there?
It looks good in the style guide

Screen.Recording.2023-11-08.at.09.59.05.mov

@susnux here's a fix for the misaligned button

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TextInput is smaller than Multiselect
5 participants