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

[Cases] UI validation for total number of comment characters #161357

Merged
merged 5 commits into from
Jul 10, 2023

Conversation

js-jankisalvi
Copy link
Contributor

@js-jankisalvi js-jankisalvi commented Jul 6, 2023

Summary

This PR adds UI validation for comments maximum length.
It shows error message and disables save button when the comment exceeds 30k characters while

  • Adding a new comment

image

  • Updating an existing comment

image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Release Notes

The total number of characters per comment is limited to 30000

@js-jankisalvi js-jankisalvi added release_note:breaking Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.10.0 labels Jul 6, 2023
@js-jankisalvi js-jankisalvi self-assigned this Jul 6, 2023
@js-jankisalvi js-jankisalvi requested a review from a team as a code owner July 6, 2023 14:17
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops-cases (Feature:Cases)

Copy link
Contributor

@adcoelho adcoelho left a comment

Choose a reason for hiding this comment

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

Tested and works fine 👍 Left a couple of comments.

@@ -174,6 +175,9 @@ export const AddComment = React.memo(
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [focusOnContext]);

const isDisabled =
isLoading || !comment?.trim().length || comment.trim().length > MAX_COMMENT_LENGTH;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not disable the button if the user does not touch the form, right? You have to use form.isValid to check if the user has interacted with the form. If undefined then the form validation has not been checked yet. We can do it when we address #161461.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #15 / Machine Learning jobs update groups "before all" hook for "returns expected list of groups after update"

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
cases 421.7KB 421.9KB +193.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
cases 146.0KB 146.0KB -28.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 14 16 +2
securitySolution 410 414 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 15 17 +2
securitySolution 489 493 +4
total +6

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @js-jankisalvi

@adcoelho adcoelho merged commit d8c8b7b into elastic:main Jul 10, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 10, 2023
@adcoelho adcoelho added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:breaking labels Jul 10, 2023
@js-jankisalvi js-jankisalvi added release_note:breaking and removed release_note:skip Skip the PR/issue when compiling release notes labels Aug 16, 2023
@js-jankisalvi js-jankisalvi deleted the ui-limit-comments-validation branch February 19, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Cases Cases feature release_note:breaking Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants