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

docs(ktextarea): to clarify maximum characters of ktextarea for readers #2484

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Miltonbhowmick
Copy link

it gives hint of maximum characters for textarea by default.

Summary

This PR enhances the docs of KTextArea by adding a note of default maximum 2048 characters limit.

it gives hint of maximum characters for textarea by default.
@Miltonbhowmick Miltonbhowmick requested a review from a team as a code owner October 28, 2024 18:35
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

netlify bot commented Oct 28, 2024

Deploy Preview for kongponents-sandbox ready!

Name Link
🔨 Latest commit 023bb9b
🔍 Latest deploy log https://app.netlify.com/sites/kongponents-sandbox/deploys/671fd9700856300008ab29fe
😎 Deploy Preview https://deploy-preview-2484--kongponents-sandbox.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -1,6 +1,9 @@
# TextArea

KTextArea provides a wrapper around textarea element, as well as contextual styling and states (error, focus, etc).
:::tip NOTE
Copy link
Member

Choose a reason for hiding this comment

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

The limit is already documented in the characterLimit prop docs; is this sufficient? Or are you suggesting it needs to more prominent?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why do we have this default limit? It seems to be uncommon so I think it’s reasonable to make it a little more prominent.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC it’s a limitation on some arbitrary endpoint we own or maybe our “normal” threshold. I don’t mind adding the extra note; we should just update this change to include a link to the corresponding prop in the note

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it’s better to remove this default limit in the future and make it injectable. Then we can provide meaningful defaults in our AppShell.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the feedback, @adamdehaven and @Justineo.
I agree that making the character limit more prominent in the documentation could benefit users, especially since this limit might be unexpected. Even if it’s standard in this system or based on specific endpoint constraints, documenting it directly in the main description section (alongside the characterLimit prop link) would ensure users understand it early, without needing to search for details in the prop list.

Adding this information up front also helps make it clear that the default limit could be adjusted in the future, as discussed. This would set the stage for a more flexible limit if we later decide to make it configurable.

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.

4 participants