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 checkbox layout #2182

Merged
merged 2 commits into from
Dec 7, 2023
Merged

Fix checkbox layout #2182

merged 2 commits into from
Dec 7, 2023

Conversation

artonge
Copy link
Collaborator

@artonge artonge commented Dec 6, 2023

Before After
image image
image image

Issue due to an internal change in @nc/vue

@artonge artonge self-assigned this Dec 6, 2023
@artonge artonge added bug Something isn't working 3. to review Waiting for reviews javascript Javascript related ticket labels Dec 6, 2023
@artonge artonge added this to the Nextcloud 29 milestone Dec 6, 2023
@artonge artonge requested review from a team, szaimen, sorbaugh and emoral435 and removed request for a team December 6, 2023 15:09
@backportbot-nextcloud backportbot-nextcloud bot added the backport-request Pending backport by the backport-bot label Dec 6, 2023
@artonge
Copy link
Collaborator Author

artonge commented Dec 6, 2023

/compile /

@susnux
Copy link
Contributor

susnux commented Dec 6, 2023

Why not dropping the :deep styling and instead use aria-label for the label. Because setting is to display: none will make it inaccessible and will be even worse.

So instead add the label as aria label here:

<NcCheckboxRadioSwitch v-if="allowSelection"

Signed-off-by: Louis Chemineau <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
Signed-off-by: Louis Chemineau <[email protected]>
@@ -335,14 +334,14 @@ export default {
width: fit-content;

// Make the checkbox background round on hover.
Copy link
Contributor

Choose a reason for hiding this comment

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

Right here, a nitpick is that now we want the background to always be present for accessibility reasons, no? So the comment could remove the hover part

Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

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

With the changes suggested by @susnux and just the nitpick at the comment, everything looks good to me

@artonge artonge merged commit 559933f into master Dec 7, 2023
23 checks passed
@artonge artonge deleted the artonge/fix/checkbox_layout branch December 7, 2023 00:39
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 backport-request Pending backport by the backport-bot bug Something isn't working javascript Javascript related ticket
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants