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 Repo Link & Add Comment Length Validation #453

Merged
merged 20 commits into from
Jun 6, 2019
Merged

Conversation

dternyak
Copy link
Collaborator

@dternyak dternyak commented Jun 6, 2019

Closes #444
Part of #405

What this does

  1. Corrects a link to point exactly to our GitHub issues
  2. Adds validation to prevent text input beyond 1000 characters to the comment box.

What this doesn't do

User feedback related to (2) is minimal, the textarea simply doesn't allow user to type additional characters. In the future, this could be improved by adding alerts, etc. Because this is handled by react-mde directly, we'd need to build our own checker outside of react-mde to determine when we should should these error messages.

Copy link
Collaborator

@wbobeirne wbobeirne left a comment

Choose a reason for hiding this comment

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

LGTM 👍

How would you feel about bumping up from 1k though? Does seem a bit small in retrospect.

@dternyak
Copy link
Collaborator Author

dternyak commented Jun 6, 2019

I think 1K is reasonable. If we get user feedback, we can increase it.

@dternyak dternyak merged commit d5ed370 into develop Jun 6, 2019
@dternyak dternyak deleted the link-and-comment branch June 6, 2019 17:28
@tromer
Copy link

tromer commented Jun 6, 2019

We're already seeing comments with 783 characters (see https://grants.zfnd.org/proposals/915080287-crypto-camp-in-the-south-bronx and unfortunately I can't link directly to the comment), so a 1000-character limit is definitely too low.

@dternyak
Copy link
Collaborator Author

dternyak commented Jun 6, 2019

Thanks for the feedback @tromer!

We'll bump this up to 5,000 and can increase further if necessary.

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.

Fix repo link for reporting issues
3 participants