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

Add headline validation error message if it reaches the maximum length #1127

Merged
merged 4 commits into from
Jul 19, 2023

Conversation

Georges-GNM
Copy link
Contributor

@Georges-GNM Georges-GNM commented Jul 17, 2023

Co-authored-by: Samantha Gottlieb samantha.gottlieb@guardian.co.uk

What does this change?

Adds a validation message to the headline field alerting users when they've reached the maximum number of characters. Due to youtube content rules, there is currently a limit of 100, but until now this wouldn't have been clear to users if they reached it.

Just to note, other fields such as the standfirst have their own erroring logic; wary of this clashing with the changes from this PR, we've restricted this validation to the Headline field.

How to test

Running MAM and entering 100 characters (or trying to enter more) in the headline field should now show an error stating You have reached the maximum character length

Images

image

Co-authored-by: Samantha Gottlieb <samantha.gottlieb@guardian.co.uk>
@Georges-GNM Georges-GNM marked this pull request as ready for review July 17, 2023 17:19
@Georges-GNM Georges-GNM requested a review from a team as a code owner July 17, 2023 17:19
Copy link
Contributor

@jonathonherbert jonathonherbert left a comment

Choose a reason for hiding this comment

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

Looks good! One comment to address before merging.

Stretch goal for a future PR – I think it'd be nice to surface the fact that a user is getting close to the limit, so they can adjust accordingly as they type, rather than bumping into the limit abruptly.

We could surface that when the field is at e.g. 2/3rd of the limit as 67/100 characters remaining as a yellow warning underneath the field.

return false;
}

if (name === "Headline" && fieldValueTooLong()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we should be singling out "Headline" in validateField, which is a very generic function.

Instead, perhaps we can act on a maxLength if the value is supplied?

Copy link
Contributor Author

@Georges-GNM Georges-GNM Jul 19, 2023

Choose a reason for hiding this comment

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

Yeah, we did have only the maxLength (which comes from videoEditValidation.js) as a check at first, but there are other fields with that parameter (e.g. the description and trail text ones) which, due to being Rich Text fields, have some extra validation logic which considers the word length. We didn't want to have these two pieces of logic to clash for those fields, hence the singling out the headline field.

For those fields which use the RichTextField.tsx component, maybe we could consider not passing the maxLength value to the ManagedField component (which means it wouldn't get passed to validateField), and instead passing maxLength directly to RichTextField, which should still enable it to be used by the validation process happening in richTextHelpers?

@Georges-GNM
Copy link
Contributor Author

Have made some changes after quickly chatting to @jonathonherbert - think this looks ok now!

Stretch goal for a future PR – I think it'd be nice to surface the fact that a user is getting close to the limit, so they can adjust accordingly as they type, rather than bumping into the limit abruptly.

We could surface that when the field is at e.g. 2/3rd of the limit as 67/100 characters remaining as a yellow warning underneath the field.

Yeah, that's a good shout, and probably fairly straightforward to implement. I'd had a similar train of thought, but along the lines of the form wouldn't actually stop the user from entering more characters in the field, but the application would just not allow you to publish - but that's probably more complicated than it needs to be!

Copy link
Contributor

@jonathonherbert jonathonherbert left a comment

Choose a reason for hiding this comment

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

Ace – tested locally, headline field has a limit as expected, and other rich text fields behave correctly for char length and word-char length behaviour 👍

@Georges-GNM Georges-GNM merged commit 2c69281 into main Jul 19, 2023
@Georges-GNM Georges-GNM deleted the gl/add-headline-validation-message branch July 19, 2023 15:23
@prout-bot
Copy link

Seen on PROD (merged by @Georges-GNM 6 minutes and 30 seconds ago) Please check your changes!

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

Successfully merging this pull request may close these issues.

3 participants