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

[#12455] Limit word and character count for text editor. #12503

Merged
merged 22 commits into from Aug 3, 2023

Conversation

rexong
Copy link
Contributor

@rexong rexong commented Jul 2, 2023

Fixes #12455

Outline of Solution
Problem:
For all rich-text-editor, the user can enter unlimited text.

Solution:
Limit the amount of text the user can enter.

  • I have limited the amount of text the user can enter by providing a limit for the word count.
  • For word count, I placed a limit of 500 words per rich-text-editor.
  • When the word limit is up, the user will not be able to enter any more words.
  • I used the wordcount plugin API from TinyMCE to get the word count that the user has entered.
    • I check the count with the limit at every key press and disable the key press once the limit is reached.
  • I have also added a line below the rich-text-editor to show how many words are left.
    • This line would only be shown if rich-text-editor is enabled.

Screenshot:
image

@rexong rexong marked this pull request as draft July 2, 2023 13:13
@rexong rexong marked this pull request as ready for review July 9, 2023 07:29
@rexong
Copy link
Contributor Author

rexong commented Jul 9, 2023

Ready for review

@weiquu
Copy link
Contributor

weiquu commented Jul 10, 2023

Hi @rexong, would it be possible to change it from a word limit to a character limit? Sorry for not pointing this out earlier on the original issue!

The reasoning is:

  • The word limit might not prevent the bloating of DB storage since users can enter characters without spaces, and that will still be considered as a single word
  • As far as I'm aware, other components use a character limit and not a word limit, so keeping to a character limit would ensure consistency

My apologies again for not flagging this out sooner!

I've also tested out your current solution, and I was able to save a response of over 500 words long... the screenshot below is after saving and refreshing the page:
Screenshot 2023-07-10 at 8 32 29 PM

As a final comment, do make the changes only to the "Instructions" field in creating and editing a session, as mentioned in the issue. Setting the limit to every single text editor raises several other issues that we probably want to avoid. For example, an instructor might want to set the recommended word count for a question to be 1000 words. However, then students would be unable to hit that recommended word count.

Do let me know if you have any questions about the above.

@rexong
Copy link
Contributor Author

rexong commented Jul 16, 2023

I have made the following changes.

  1. Changed word count to character count.
    • The character count is set to 2000.
  2. Handle the character limit when users paste their clipboard text into the text editor.
  3. Only affects the "Instruction" field in creating and editing a session, any other places that uses the text editor would not be affected.

Ready for Review!

Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the changes (:

@weiquu weiquu added the s.FinalReview The PR is ready for final review label Jul 16, 2023
Copy link
Contributor

@domlimm domlimm left a comment

Choose a reason for hiding this comment

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

Nice work! Just a minor comment. Would like both of your thoughts @rexong @weiquu

If we copy the text (via Select All and Copy) we've already entered in the editor and paste it back, the number of characters left is back to 2000.

If we cut the text instead, everything works as expected. Not sure if it's due to TinyMCE's API/the intended behaviour.

A video for both of your reference @ x2 speed please. Sorry it's a little lengthy, was thinking what to write and also show the behaviour.

2023-07-16.23-27-07.mp4

@rexong
Copy link
Contributor Author

rexong commented Jul 17, 2023

@domlimm Thank you for pointing this out. I will look into this issue.

@rexong
Copy link
Contributor Author

rexong commented Jul 23, 2023

I have looked into the issue. It was because of my implementation of the paste event. For the paste event, I have set a timeout to ensure that the paste event has been executed before any characters count updates. But there is no timeout when I use the GetContent event. When a user selects all and paste, the paste event will empty the textbox before pasting the content. Without the timeout, GetContent would only fetch the empty textbox content, not the content after the paste event. I have added the timeout to the GetContent event to account for the paste event as well.

New.video.mp4

@weiquu weiquu requested a review from domlimm July 23, 2023 09:21
@weiquu
Copy link
Contributor

weiquu commented Jul 23, 2023

Hi @rexong, sorry just noticed something else - the line breaks seem to disappear after the paste event has been stopped. I entered some text and some line breaks in between, and then copied the highlighted text:
Screenshot 2023-07-23 at 5 28 44 PM

After pasting the highlighted text at the end, this is the result (notice the line breaks are gone, and the count still shows characters left):
Screenshot 2023-07-23 at 5 28 54 PM

I believe it should be an issue with the editor.setContent(limitContent); line, though I'm not very certain.

Also noted a second issue: even if the paste is in the middle of the text, the substring that is removed is from the end of the text. For example, I've entered some text here and copied the "questions" word:
Screenshot 2023-07-23 at 5 35 16 PM

After pasting the "questions" word into the whitespace in the centre, the text at the end (i.e. "hello") is gone:
Screenshot 2023-07-23 at 5 35 34 PM

I'm wondering if this behaviour would be expected by the user... I would think that the behaviour should be the pasted text gets truncated instead. If you agree, my suggestion would be to get the substring of the pasted text instead and paste it at its intended location. This might solve the first issue as well since we're not setting the content of the whole editor (though I'm not too sure).

Copy link
Contributor

@domlimm domlimm left a comment

Choose a reason for hiding this comment

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

Nice work! Thank you for your contribution!

@domlimm domlimm added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Jul 24, 2023
@domlimm domlimm added s.OnHold The issue/PR's validity has been put on hold pending some other event and removed s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging labels Jul 24, 2023
@domlimm
Copy link
Contributor

domlimm commented Jul 24, 2023

@rexong @weiquu

I would think that the behaviour should be the pasted text gets truncated instead.

I will stick with this, it makes more sense. If I am using this editor and my end of text gets truncated, it could give a bad user experience.

HW, let us know your thoughts. Always welcome other views! Not sure on the complexity of this implementation. But yes substring is the way to go as WQ suggested. I think we can close an eye on the performance for now as it's capped at 2KB worth of characters, performance shouldn't be impacted, is significant.

@domlimm domlimm added s.ToReview The PR is waiting for review(s) and removed s.OnHold The issue/PR's validity has been put on hold pending some other event labels Jul 24, 2023
@rexong
Copy link
Contributor Author

rexong commented Jul 25, 2023

@domlimm @weiquu
To address WQ's first issue, TinyMCE editor will parse the content in the editor as HTML. Here is an example of the content,
image
To trim the content to the right length, I used getContent({ format: 'text' }) to get the plain text and trim the excess off. Then, when using setContent(), I set the content based on the plain text. This is probably why the empty spaces are removed.

To address the second issue, I am not too sure if there is a way for me to just get the paste content/ substring. I could probably compare the content before and after the paste event?

@weiquu
Copy link
Contributor

weiquu commented Jul 26, 2023

Thanks @rexong for looking into the cause of the issue. I've tried some stuff but am unable to find a solution for the first issue... @domlimm any suggestions? If not, we can open a separate issue for it once this PR gets merged. Should also note that all other formatting (e.g. bold, strikethrough) also gets removed.

For the second part on the truncation of the pasted text, I've done up a quick snippet to see if it's possible:

const before = editor.getContent({ format: 'text' });
setTimeout(() => {
    const currentCharacterCount = this.getCurrentCharacterCount(editor);
    if (currentCharacterCount >= RICH_TEXT_EDITOR_MAX_CHARACTER_LENGTH) {
        event.preventDefault();
        const after = editor.getContent({ format: 'text' });
        var firstDifferentIndex = 0;
        while (after[firstDifferentIndex] === before[firstDifferentIndex]) {
            ++firstDifferentIndex;
        }
        const left = before.substring(0, firstDifferentIndex);
        const right = before.substring(firstDifferentIndex);
        const pastedTextLength = after.length - before.length;
        const pastedText = after.substring(firstDifferentIndex, firstDifferentIndex + pastedTextLength);
        const amountOver = currentCharacterCount - RICH_TEXT_EDITOR_MAX_CHARACTER_LENGTH;
        const truncatedPastedText = pastedText.substring(0, pastedTextLength - amountOver);
        const finalText = left + truncatedPastedText + right;
        editor.setContent(finalText);

I ran a couple of tests and it seems to work, @rexong can I leave it to you to make sure that it works with any edge cases as well? I also didn't get to the part on setting the cursor; the cursor should probably be set to the end of the (truncated) pasted text.

@rexong
Copy link
Contributor Author

rexong commented Jul 31, 2023

Hello @weiquu, thanks for the code snippet. I have added and tested the implementation. It seems to be working fine. In addition, I made some edits to set the cursor as well. Now the cursor will be set to wherever the pasted text ends when the character limit is reached.

Here's a demo.

TEAMMATES.-.Online.Peer.Feedback_Evaluation.System.for.Student.Team.Projects.-.Google.Chrome.2023-07-31.22-57-26.mp4

@weiquu weiquu requested review from weiquu and domlimm August 2, 2023 07:11
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

LGTM! Pinging @domlimm to double check the change

@weiquu weiquu added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Aug 2, 2023
@domlimm
Copy link
Contributor

domlimm commented Aug 2, 2023

Will review by tonight.

Copy link
Contributor

@domlimm domlimm left a comment

Choose a reason for hiding this comment

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

LGTM! Good job well done! Thank you for your efforts @rexong!

Sorry again for all the delays... Will merge once the checks pass.

@jasonqiu212 jasonqiu212 merged commit 0976824 into TEAMMATES:master Aug 3, 2023
9 checks passed
@domlimm
Copy link
Contributor

domlimm commented Aug 3, 2023

@jasonqiu212 thanks Jason! 😄

@samuelfangjw samuelfangjw added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Feature User-facing feature; can be new feature or enhancement to existing feature and removed s.FinalReview The PR is ready for final review labels Aug 21, 2023
@samuelfangjw samuelfangjw added this to the V8.29.0 milestone Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Feature User-facing feature; can be new feature or enhancement to existing feature s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit for "Instructions" field in creating and editing a session not correctly treated.
5 participants