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
Comments
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. But as with the initial discussion, this is a design decision so cc @jancborchardt @nimishavijay @marcoambrosini |
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. |
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) |
@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. |
But for AAA. But also important: Unify element height.
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. But as I said we should unify the height. NcSelect is even more height. |
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. If we really want to move to the inner label, I think we should go above 50px height like in the previous examples. |
It looks better now, but I still think the other option looks better while using considerably less space. |
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 👍 |
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 |
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. (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) |
By the way, does the current solution fit AAA requirements about text readability, text height/spacing? |
By the current I meant - in the |
No, because of this problem: nextcloud/server#36977 (comment) |
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? |
This part is already custom styled, the input was sized to 44px instead of 36px and made much rounder |
@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.
The height of the search conversation input is 36px, custom style is the pill border
@JuliaKirschenheuter the label needs to be vertically centered in the input when the input is not active |
...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. |
Hi @marcoambrosini, Thank you for your anwer! But i disageree with you in that point:
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. |
Could we please transfer it to another improvement issue? |
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:
|
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 |
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.
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. |
Ok, which way should be finally go? |
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. |
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? |
@marcoambrosini can we close this issue as #4718 is merged? |
i guess we don't need it as last decision was to follow this way #4582 (comment) |
Yes! |
With new input labels, there are two design problems:
There was another proposal for a visible label, described in #4287, that was closed in favor of #4394
Having these issues with MD-like labels in apps, what do you think about considering returning to the first proposal?
The text was updated successfully, but these errors were encountered: