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(NcCheckboxRadioSwitch): Pass attrs to input if available #5507

Merged
merged 1 commit into from
Apr 20, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Apr 19, 2024

☑️ Resolves

I would expect the attrs on the input element to allow any aria attributes.
So if not button type, pass the attrs to the input element to allow setting aria tags like aria-invalid and aria-errormessage

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@susnux susnux added bug Something isn't working 3. to review Waiting for reviews feature: checkbox-radio-switch Related to the checkbox-radio-switch component labels Apr 19, 2024
@susnux
Copy link
Contributor Author

susnux commented Apr 19, 2024

/backport to next

If not button type, pass the attrs to the input element to allow setting aria tags like `aria-invalid` and `aria-errormessage`

Signed-off-by: Ferdinand Thiessen <[email protected]>
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.

Why don't pass to the button as well?

@susnux
Copy link
Contributor Author

susnux commented Apr 20, 2024

Why don't pass to the button as well?

They are as in this case it is the root element.

@susnux
Copy link
Contributor Author

susnux commented Apr 20, 2024

we maybe should add consistent props for all input elements to have some focus and validation API independent of the implementation.

@susnux susnux merged commit d6d00ca into master Apr 20, 2024
18 checks passed
@susnux susnux deleted the fix/checkbox-attrs branch April 20, 2024 15:39
Comment on lines +329 to +330
// We need to pass attributes to the input element
inheritAttrs: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

They are as in this case it is the root element.

But it is disabled via inheritAttrs: false here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For vue2 class and style are passed to the root as they are not attrs but somewhat special

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't pass to the button as well?

Here I meant the attrs, not only class/style.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently there is a new possibility to pass attrs to input, but the possibility to pass attrs to the button has been disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So class & style are always set to the wrapper.
attrs are passed to the wrapper (the button) for button type and to the input for other types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry, missed this line.

@skjnldsv
Copy link
Contributor

skjnldsv commented Jul 3, 2024

Breaks server nextcloud/server#46239
we can't apply attributes to the outer span now, that is an issue imho

@susnux
Copy link
Contributor Author

susnux commented Jul 4, 2024

we can't apply attributes to the outer span now, that is an issue imho

The structure of components is private and can change anytime, thus assumptions about it will always be prone to break.
That is why we have e.g. content-classes etc for assigning custom classes to dialogs without any assumption about internal class names or structure.
In this case you have a checkbox thus you would expect to have a checkbox element (input).

This "regression" is only about tests that assume some specific internal structure, but it is more a fix.

If you currently have cy.get('[data-cy-my-checkbox] input') you could simplify to cy.get('[data-cy-my-checkbox]') or the most secure way: cy.findByRole('checkbox', { name: 'My checkbox' }) which will work regardless of the internal structure and only depends on your app code (the accessible name that you assign).

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 bug Something isn't working feature: checkbox-radio-switch Related to the checkbox-radio-switch component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants