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

[RFC]: NcInputField - visible inner label inside input #4582

Closed
ShGKme opened this issue Sep 26, 2023 · 39 comments
Closed

[RFC]: NcInputField - visible inner label inside input #4582

ShGKme opened this issue Sep 26, 2023 · 39 comments
Assignees
Labels
discussion Need advices, opinions or ideas on this topic feature: input-field Covering the InputField, TextField, ...

Comments

@ShGKme
Copy link
Contributor

ShGKme commented Sep 26, 2023

With new input labels, there are two design problems:

  1. It has a lower height to have space for a label on the border. It makes it smaller than other clickable elements, smaller than the required minimal clickable area, and not in line with buttons and other clickable elements:
    image
  2. It doesn't look good on different backgrounds, which makes it not recommended to use when the background doesn't match (described in fix(NcInputField): Adjust styling of the internal label #4578)
    image

There was another proposal for a visible label, described in #4287, that was closed in favor of #4394

image

Having these issues with MD-like labels in apps, what do you think about considering returning to the first proposal?

@ShGKme ShGKme added discussion Need advices, opinions or ideas on this topic feature: input-field Covering the InputField, TextField, ... labels Sep 26, 2023
@susnux
Copy link
Contributor

susnux commented Sep 26, 2023

I personally like both versions.

But I think the first approach is less error prone, as you do not have to think about any problems with the background where you use the components.
Moreover the outline / border-box is more clear as what you see is the outline and the label is only moved within the borders.


But as with the initial discussion, this is a design decision so cc @jancborchardt @nimishavijay @marcoambrosini

@jancborchardt
Copy link
Contributor

the first approach is less error prone, as you do not have to think about any problems with the background where you use the components.

Agree with this assessment @susnux, and thanks @ShGKme for bringing this up. I would agree with switching to the first proposal as it will look much better in all sorts of environments.

@marcoambrosini
Copy link
Contributor

Back then I raised concerns about the readability of the text. The label is almost tangent to the border and might break line height accessibility requirements. And padding around input text is very little. I think that the readability of both input text and label text is more important than this background edge case.

@jancborchardt You can get a sense for it in the action menu here #4287 (comment)

@jancborchardt
Copy link
Contributor

@ShGKme @JuliaKirschenheuter do we know how the proposed solution would fare accessibility-wise?

@ShGKme
Copy link
Contributor Author

ShGKme commented Oct 5, 2023

@ShGKme @JuliaKirschenheuter do we know how the proposed solution would fare accessibility-wise?

It would be even better because currently clickable area of inputs is smaller than the required 44px.

Apart from that, there is no difference, it's only about input height and label position change for about 4px.

@marcoambrosini
Copy link
Contributor

marcoambrosini commented Oct 5, 2023

The clickable area of elements doesn't have to be 44px to meet the aria AA target size success criterion. The line height of the text needs to be at least 1.5 though and putting a border at the very end of the line height looks wrong to me anyway.

This is how proper spacing around text looks in a contained label text input:

Airbnb
Screenshot 2023-10-06 at 08 38 33

Material

Screenshot 2023-10-06 at 08 43 58

Both are over 50px to provide the necessary whitespace around the text. By contrast, this is our component:

Screenshot 2023-10-06 at 08 45 30

I leaned towards the other version because whilst using less space, it provides better readability of the text.

Screenshot 2023-10-06 at 08 50 14

@susnux
Copy link
Contributor

susnux commented Oct 6, 2023

The clickable area of elements doesn't have to be 44px to meet the aria AA target size success criterion.

But for AAA. But also important: Unify element height.

The line height of the text needs to be at least 1.5 though

No it is not necessary, WCAG only defines this gor text blocks to help reading it. For this element we just need to ensure the padding to the bottom.

So 13px label + 7px padding + 15px text = 35px height.
So there are still 4px for borders (= 39px) and 5px additional padding for label top and bottom.

But as I said we should unify the height. NcSelect is even more height.

@marcoambrosini
Copy link
Contributor

marcoambrosini commented Oct 6, 2023

But for AAA

We are aiming at AA as far as I understand. The button component is an exception.

There is no need to unify the height of buttons and inputs. It might be convenient, yes, but it's by no means a priority. So I don't agree that we should unify the height.
Text readability is far more important. No major UI library seems to be bothered with equalizing button height with input height.

If we really want to move to the inner label, I think we should go above 50px height like in the previous examples.

@susnux
Copy link
Contributor

susnux commented Oct 12, 2023

We can unify with NcSelect, then it even works for a11y (screenshot was taken with 1.5 arctoolkit so it looks a bit off but it shows that is succeeds the tests):
Screenshot_20231012_172812

This has a height of 48px like NcSelect. This way it looks consistent when using input + select.
And even fit in the table rows (files etc) which have a height of 50px.

@marcoambrosini
Copy link
Contributor

It looks better now, but I still think the other option looks better while using considerably less space.
Let's wait for another @jancborchardt @szaimen @nimishavijay to read this thread and share their thoughts.

@susnux
Copy link
Contributor

susnux commented Oct 13, 2023

Let's wait for another to read this thread and share their thoughts.

Sure :)

For testing you can find the old proposal with the inner label in branch feat/nciputfield-v2

[...] while using considerably less space.

BTW the reason why I prefer unify the height of input elements are forms where you have multiple components, they look odd if the height does not match:

Screenshot_20231013_182957 Screenshot_20231013_183157

(Of course the inner label is missing on the select, but even if it would also support it, the height would not match.)

@JuliaKirschenheuter
Copy link
Contributor

Dear @jancborchardt / @marcoambrosini, @nimishavijay,

i would vote for unifying the height of input elements. Would you have any points against that?

Dear design team, we have to move forwards on our a11y issues and need a decision asap. Please give us a feedback how could we move forwards 🙏. Thanks a lot!

@szaimen
Copy link
Contributor

szaimen commented Oct 20, 2023

Dear @jancborchardt / @marcoambrosini, @nimishavijay,

i would vote for unifying the height of input elements. Would you have any points against that?

Dear design team, we have to move forwards on our a11y issues and need a decision asap. Please give us a feedback how could we move forwards 🙏. Thanks a lot!

I still think that having thr label in the border looks better but of coure having the same height for select and input field also makes sense. I guess we cannot reduce the height of the select component to match the input field? If so I am fine with this change 👍

@ShGKme
Copy link
Contributor Author

ShGKme commented Oct 20, 2023

I still think that having thr label in the border looks better

In this case, how should be work with different backgrounds? For example, on the login page (not the only one example).

@szaimen
Copy link
Contributor

szaimen commented Oct 20, 2023

In this case, how should be work with different backgrounds? For example, on the login page (not the only one example).

There a input field with label outside should be used IIRC. cc @nextcloud-libraries/designers on this

@susnux
Copy link
Contributor

susnux commented Oct 20, 2023

If we make NcSelect smaller, we do not gain anything. It still looks weird then because the outline might match but the label on the border will not.

I think the simplest solution would be the label within and make input fields the same height as NcSelect.
This way we solve multiple problem (background colors, height issues, text height issues, clickable area for AAA) and it will look better even with a label next to a NcSelect.

(I think the problem from design perspective (at least for me) is that the label on the border is a new design element and will always look odd compared to other components, either they need this option too or no border label is used when next to an other element. The inner label does not have this problem because the border is "not broken" so if both are the same height they still match visually)

@ShGKme
Copy link
Contributor Author

ShGKme commented Oct 20, 2023

By the way, does the current solution fit AAA requirements about text readability, text height/spacing?

@JuliaKirschenheuter
Copy link
Contributor

By the way, does the current solution fit AAA requirements about text readability, text height/spacing?

I'm not an expert, but that seems (okay). Could be better, but it is readable:

Screenshot from 2023-10-23 09-19-32

@JuliaKirschenheuter
Copy link
Contributor

But there is some other problem: if label is not visible (case where input field is empty):

Screenshot from 2023-10-23 09-26-08

@ShGKme
Copy link
Contributor Author

ShGKme commented Oct 23, 2023

By the way, does the current solution fit AAA requirements about text readability, text height/spacing?

I'm not an expert, but that seems (okay). Could be better, but it is readable:

By the current I meant - in the @nextcloud/vue's master

@JuliaKirschenheuter
Copy link
Contributor

By the current I meant - in the @nextcloud/vue's master

No, because of this problem: nextcloud/server#36977 (comment)

@marcoambrosini
Copy link
Contributor

We just talked about this in the design meeting and we will go with the last version by @susnux (48px) :)

@JuliaKirschenheuter
Copy link
Contributor

@marcoambrosini, @jancborchardt, @nimishavijay

would you approve a current state?
Peek 2023-10-24 15-28

@ShGKme
Copy link
Contributor Author

ShGKme commented Oct 24, 2023

We just talked about this in the design meeting and we will go with the last version by @susnux (48px) :)

@marcoambrosini Do you think we should make an optional behavior to return the previous or "small" layout?

Or it fits fine most of the usages? I'm a bit afraid of layouts that might have counted on this size.

For example, what this place in apps' navigation should look like?

image

@susnux
Copy link
Contributor

susnux commented Oct 24, 2023

For example, what this place in apps' navigation should look like?

This part is already custom styled, the input was sized to 44px instead of 36px and made much rounder

@marcoambrosini
Copy link
Contributor

marcoambrosini commented Oct 25, 2023

Do you think we should make an optional behavior to return the previous or "small" layout?

@ShGKme I think that by doing this we're introducing more problems that we're solving, we're suddenly increasing the height of this element by 12 pixels. @jancborchardt I leave this in your hands for further decisions on this as I just don't see the point of this change.
There's no accessibility issue whatsoever with the current input, just saying this for the record because it seemed yesterday that @jancborchardt and @JuliaKirschenheuter thought there were.
The design team couldn't explain yesterday why the select component is suddenly so tall, so "matching the select height" becomes even less of a reason for doing this.

This part is already custom styled, the input was sized to 44px instead of 36px and made much rounder

The height of the search conversation input is 36px, custom style is the pill border

Would you approve a current state?

@JuliaKirschenheuter the label needs to be vertically centered in the input when the input is not active

@marcoambrosini
Copy link
Contributor

marcoambrosini commented Oct 25, 2023

...actually, here's one last proposal keeping the current style but bringing it up to 44px (input size not counting label) and bringing the select height back down to 44px too.
As said yesterday, forms should in general be on "main" backgrounds and the login is an edge case that can be handled separately. I included a "darker" background version which doesn't look bad at all.

Screenshot 2023-10-25 at 14 54 55

@JuliaKirschenheuter
Copy link
Contributor

Hi @marcoambrosini,

Thank you for your anwer! But i disageree with you in that point:

There's no accessibility issue whatsoever with the current input, just saying this for the record because it seemed yesterday that @jancborchardt and @JuliaKirschenheuter thought there were.

Please look in the truncation problem here nextcloud/server#36977 (comment). And this is an accessibility problem. It must be checked whether changing text spacing leads to text truncation, overlap, or loss of functionality.

@JuliaKirschenheuter
Copy link
Contributor

...actually, here's one last proposal keeping the current style but bringing it up to 44px (input size not counting label) and bringing the select height back down to 44px too. As said yesterday, forms should in general be on "main" backgrounds and the login is an edge case that can be handled separately. I included a "darker" background version which doesn't look bad at all.

Screenshot 2023-10-25 at 14 54 55

Could we please transfer it to another improvement issue?

@marcoambrosini
Copy link
Contributor

Please look in the truncation problem here nextcloud/server#36977 (comment)

That is a separate issue @JuliaKirschenheuter and has nothing to do on whether we go with contained label vs label on border

@JuliaKirschenheuter
Copy link
Contributor

That is a separate issue @JuliaKirschenheuter and has nothing to do on whether we go with contained label vs label on border

It has. Please compare truncation:

After Before
image image

@susnux
Copy link
Contributor

susnux commented Oct 25, 2023

As said yesterday, forms should in general be on "main" backgrounds and the login is an edge case that can be handled separately.

That is not fully true, yes the login is - together with the dashboard widgets - an edge case. But we quite often have at least a darker background. E.g. the hover state (user list rows) etc.

So we are not really independent to the background

@marcoambrosini
Copy link
Contributor

It has. Please compare truncation:

There's some container clipping going on there and this issue is really not about that @JuliaKirschenheuter. That can happen with both designs and can be solved independently from the path we take.

E.g. the hover state

Text inputs should not be within elements that one can click on in the first place, so there should be no hover state background change surrounding them.

@JuliaKirschenheuter
Copy link
Contributor

Ok, which way should be finally go?

@marcoambrosini
Copy link
Contributor

I'm stepping back for good after that final proposal, Jan will weigh in with a final decision soon. The truncation of the component can be solved regardless of the outcome of the issue.

@jancborchardt
Copy link
Contributor

Ok, spacing-wise, @marcoambrosini's latest proposal looks much better indeed. It's not as big and jarring. Let's go for this.

For the truncation issue, we need to make sure the label container is big enough and/or the text label is is centered. As @marcoambrosini said this can/must be solved independently of the overall solution.

@marcoambrosini can you please stay on this to help with the CSS details?

@JuliaKirschenheuter
Copy link
Contributor

@marcoambrosini can we close this issue as #4718 is merged?

@JuliaKirschenheuter
Copy link
Contributor

For testing you can find the old proposal with the inner label in branch feat/nciputfield-v2

i guess we don't need it as last decision was to follow this way #4582 (comment)

@marcoambrosini
Copy link
Contributor

@marcoambrosini can we close this issue as #4718 is merged?

Yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Need advices, opinions or ideas on this topic feature: input-field Covering the InputField, TextField, ...
Projects
None yet
Development

No branches or pull requests

6 participants