guardian / media-atom-maker

A tool for creating media atoms, including a UI specifically for managing video
https://video.gutools.co.uk/videos
4 stars 3 forks source link

Fix bug in word limit #1133

Closed davidfurey closed 3 months ago

davidfurey commented 1 year ago

What does this change?

The tooLong function currently limits the number of non-whitespace characters rather than the number of words. We think word limit is what was intended. This PR updates the tests so the existing implementation fails, and then fixes the function.

It also removes reference to character limit in the test, since this feature was removed by https://github.com/guardian/media-atom-maker/pull/1127

How to test

Tricky to test without tweaking the word limits in videoEditValidation.js, since the word limit is currently half the character limit. If you were to change the word limit to 13 words, you should see that on main the text "I would like, if I may, to take you on a strange journey." is deemed too long, but with on this branch, it is acceptable.

But there is a unit test, so might be okay to just merge as is?

How can we measure success?

No one notices. Main benefit is that when this code is copy and pasted (e.g. into prosemirror-editor) it does what developers expect.

Have we considered potential risks?

This reduces the limit (if anything it increases the limit), and editorial cannot write the content they want. We would just roll back.

rhystmills commented 1 year ago

My intuition is that a character limit is more useful in most places - if the text is destined for CAPI fields we would probably want something of relatively predictable visual length (for when it's rendered on the frontend) - if the text it's destined for YouTube it's probably got a hard character limit.

davidfurey commented 1 year ago

Unless I've misunderstood, we still have a max character limit, it is just handled elsewhere. This function used to do both, but in https://github.com/guardian/media-atom-maker/pull/1127 the character limit functionality was moved as part of adding clearer messaging

rhystmills commented 1 year ago

Ah - no, I missed that addition - ignore me!

prout-bot commented 3 months ago

Seen on PROD (merged by @davidfurey 5 minutes and 21 seconds ago) Please check your changes!