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

[BITV] 9.3.3.2/3.1 - Comment section: The input fields for creating and editing comments lack visual labels. (2) #41884

Closed
JuliaKirschenheuter opened this issue Nov 29, 2023 · 6 comments · Fixed by #42676
Assignees
Labels
1. to develop Accepted and waiting to be taken care of a11y28checked needed for a11y accessibility

Comments

@JuliaKirschenheuter JuliaKirschenheuter added 1. to develop Accepted and waiting to be taken care of accessibility labels Nov 29, 2023
@susnux susnux self-assigned this Nov 30, 2023
@ShGKme
Copy link
Contributor

ShGKme commented Dec 1, 2023

There was a suggestion to add the same label as in text fields: #37087 (comment)

However, adding such a label to NcRichcontenteditable would be kind of a "hidden" breaking change and used only here.

What about removing the user avatar here and "Add a comment" label outside here?

@susnux
Copy link
Contributor

susnux commented Dec 3, 2023

There was a suggestion to add the same label as in text fields: #37087 (comment)

I agree on this we should provide the same style of labels for all input elements, NcRichcontent... NcSelect..., like already did with NcTextarea, to have a consistent UI (from a design perspective). But that is out of scope of this issue.

However, adding such a label to NcRichcontenteditable would be kind of a "hidden" breaking change and used only here.

Why would it be a breaking change? For me it would be a feature that is disabled by default..

What about removing the user avatar here and "Add a comment" label outside here?

This would be a breaking design change an make it inconsistent with any other activity.

@ShGKme
Copy link
Contributor

ShGKme commented Dec 3, 2023

Why would it be a breaking change? For me it would be a feature that is disabled by default..

Currently NcRichContenteditable node is directly a div[contenteditable] node. To add this label, we need to change it by adding a wrapper element. It is not formally a breaking change, but it may break style overriding and direct usage with a template ref.

This would be a breaking design change an make it inconsistent with any other activity.

Adding a label is also a visible change.

But what do you mean by other activity? Is it about activities listed in this tab? If that is, should it be consistent? A form is not an activity.

@susnux
Copy link
Contributor

susnux commented Dec 3, 2023

But what do you mean by other activity? Is it about activities listed in this tab? If that is, should it be consistent? A form is not an activity.

Yes consider clicking edit on the comment, the expected behavior would be to change the comment to the editable version while keeping all other elements.

[...] direct usage with a template ref.

That depends on what you are doing with it using the ref directly is pretty dangerous anyways as you then depend on the implementation details. Meaning if we want any methods to be exposed we should use expose and document them. For example if you need to access the inner contenteditable we could document and expose the ref to contenteditable of the component.

@ShGKme
Copy link
Contributor

ShGKme commented Dec 3, 2023

Yes consider clicking edit on the comment, the expected behavior would be to change the comment to the editable version while keeping all other elements.

You are correct, I missed that.

@susnux
Copy link
Contributor

susnux commented Dec 11, 2023

Waiting for release of nextcloud-libraries/nextcloud-vue#4907

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of a11y28checked needed for a11y accessibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants