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

A11y improvment (part 2) : Customize textarea props #553

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

basile-parent
Copy link
Contributor

Second part of the improvments proposed in #544 : giving access to all content editable component props so we can customize the textbox as we want (such as putting an id, a dynamic aria-label or any a11y prop we want)

I add some examples in the "examples" folder

@basile-parent basile-parent mentioned this pull request Jul 24, 2024
2 tasks
@petyosi
Copy link
Contributor

petyosi commented Jul 24, 2024

The approach you suggest here is going to result in a breaking change. I do understand the intent here, but I would rather not make a breaking change for a relatively simple feature introduction. I'm ok with the idea of additional contentEditableProps (can even call it attributes), but there's no need to remove/alter the contentEditableClassName.

@basile-parent
Copy link
Contributor Author

Your right for breaking changes. I will get class name back.

Can I push a deprecated notice on it (as it's covered by the general "attributes" / "props") ? That way, you could delete it in few versions.

The "props" suffix seems clearer for me (as you pass props directly to this child componen) but if you think "attributes" is better, I'll follow you ^^ Do you wants just "attributes" or "contentEditableAttributes" ?

@basile-parent
Copy link
Contributor Author

@petyosi The className is back and flagged deprecated. For the naming convention, I let you decide (see previous comment)

@basile-parent
Copy link
Contributor Author

Hello @petyosi, do you think this PR (and the #552) could be merged ? Thank you :-)

@petyosi
Copy link
Contributor

petyosi commented Jul 31, 2024

@basile-parent Referring to both PRs, I'm very hesitant to merge a large change given that the final outcome should be a couple of changed attribute values.

This being said, I will try to find the time to examine the intended attribute change from your PRs and find a way to apply those changes in a less intrusive manner.

@basile-parent
Copy link
Contributor Author

OK, as you want :-) Accessibility is always a matter of few attribute values ^^

Thank you for your time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants