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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: added a label element to the NcSelect in update settings #43094

Merged
merged 1 commit into from Jan 27, 2024

Conversation

emoral435
Copy link
Contributor

Summary

@JuliaKirschenheuter (all credit to her) found that in index.php/settings/admin/overview, there is a NcSelect without a corresponding label. This fixes that, No visual change, gave the label input the corresponding styles that would accompany the change from h4 element 馃憤

TODO

  • Added a label to correspond with out input element, as well as matching their ID's

Checklist

@emoral435
Copy link
Contributor Author

Will /backport once accepted by reviews :)

@emoral435
Copy link
Contributor Author

/compile amend/

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Some nitpicks

@ShGKme
Copy link
Contributor

ShGKme commented Jan 24, 2024

No visual change, gave the label input the corresponding styles that would accompany the change from h4 element 馃憤

I think we can make it plain text. It looks inconsistent now, not sure it was needed to be h4 with h4 styles. cc @szaimen

@emoral435 emoral435 force-pushed the fix/a11y/43088/select-has-no-connected-label branch 2 times, most recently from 2d305e6 to 725ee59 Compare January 25, 2024 15:41
@emoral435 emoral435 requested a review from Pytal January 25, 2024 15:41
@emoral435
Copy link
Contributor Author

@Pytal Fixed! And styling should be the same - had to add high order specifiers with ID's and whatnot because Vue's default styling on their Select component was beating out most things I tried. This kept it pretty consistent:

firefox_3QXFPCor22

@Pytal
Copy link
Member

Pytal commented Jan 25, 2024

I'd say we should to drop the custom select__label styles and set width: fit-content on the wrapper + min-width: 100% on the dropdown toggle to have the select and label be the same width

@nextcloud/designers for design input

@emoral435 emoral435 force-pushed the fix/a11y/43088/select-has-no-connected-label branch from 725ee59 to 914d0b0 Compare January 26, 2024 16:14
@emoral435
Copy link
Contributor Author

Agreed, IMO I think that having the label be 900px wide but then having the select be way smaller is a bit weird. Will push this then try to see what the designers say. Here is how it looks like, for the @nextcloud/designers :

firefox_dTCLp67UjB

@szaimen
Copy link
Contributor

szaimen commented Jan 26, 2024

Agreed, IMO I think that having the label be 900px wide but then having the select be way smaller is a bit weird. Will push this then try to see what the designers say. Here is how it looks like, for the @nextcloud/designers :

firefox_dTCLp67UjB firefox_dTCLp67UjB

Honestly from design perspective I'd say this detail doesnt really matter as there are many other things on this page that don't look visually perfect 馃槄馃檲

@emoral435 emoral435 requested a review from Pytal January 26, 2024 17:45
@susnux susnux force-pushed the fix/a11y/43088/select-has-no-connected-label branch from 914d0b0 to 9148573 Compare January 27, 2024 01:09
@susnux
Copy link
Contributor

susnux commented Jan 27, 2024

/compile amend

@susnux susnux enabled auto-merge January 27, 2024 01:10
@susnux
Copy link
Contributor

susnux commented Jan 27, 2024

/backport to stable28

Signed-off-by: Eduardo Morales <emoral435@gmail.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@nextcloud-command nextcloud-command force-pushed the fix/a11y/43088/select-has-no-connected-label branch from 9148573 to 35518bc Compare January 27, 2024 01:14
@susnux susnux merged commit fe4dde3 into master Jan 27, 2024
95 checks passed
@susnux susnux deleted the fix/a11y/43088/select-has-no-connected-label branch January 27, 2024 01:39
@blizzz blizzz mentioned this pull request Mar 5, 2024
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.

[BITV]: Select has no connected label
5 participants