Skip to content

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

@petyosi petyosi closed this May 30, 2025
@basile-parent
Copy link
Contributor Author

Hi @petyosi, can you explain why do you close this one ? It seems pretty simple changes and, in addition to accessibility improvments, adds some flexibility for future features.

Thank you

@petyosi
Copy link
Contributor

petyosi commented Jun 12, 2025

I closed this because of the following:

I'm very hesitant to merge a large change given that the final outcome should be a couple of changed attribute values.

I do understand the necessity of aria attributes, but not with that kind of a breaking change/implementation.

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