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

💻 Automatically add curly braces for individual keywords #5285

Merged
merged 30 commits into from
Apr 16, 2024

Conversation

jpelay
Copy link
Member

@jpelay jpelay commented Mar 21, 2024

Adds a function that automatically surrounds individual keywords with curly braces son teachers don't have to, and for the cases where one individual word might match with several keywords, a warning message is displayed, asking the user to select one of the keywords that might match that one.

This video shows an example of how the alert message is displayed:

2024-03-20.20-43-44.mp4

How to test

Follow these steps to verify this PR works as intended:

  • Login as a teacher and start editing an adventure
  • Type a word that might have several keywords associated, like to in English and check that the alert message is displayed.
  • After saving the adventure, go to dev_database.json and search for the field associated to the adventure you just edited, and check that the field formatted_content correctly has curly braces surrounding the keywords

@hasan-sh
Copy link
Collaborator

hasan-sh commented Mar 26, 2024

Hello @jpelay, existing adventures may have such keywords but the warningbox isn't shown to them, one might argue that it should!
image

I think we should utilize the function you created in

window.ckEditor = editor;
$editor = editor;
resolve();

Perhaps related to this, did you check the threads starting from #5297 (comment) ?!

@hasan-sh
Copy link
Collaborator

If i add a suggested keyword without the curly braces, the keyword isn't localized correctly. Thus we need to check how you add curly braces perhaps it's not recognizing a keyword with _!
image
image

@hasan-sh
Copy link
Collaborator

Hi @jpelay , i think this PR should be of highest priority because the content of adventures are changed and teachers see curly braces. It'd be nice to finish it this week. I'll review it once you make progress and think it's ready!

@jpelay
Copy link
Member Author

jpelay commented Mar 27, 2024

Hi Hasan!

If i add a suggested keyword without the curly braces, the keyword isn't localized correctly. Thus we need to check how you add curly braces perhaps it's not recognizing a keyword with _!

This is in an interesting question. I assumed that people would want to include the keywords as they are use in real programs, to_list is not really how you write that keyword, but I think it makes sense adding the curlies around them, in case teachers have that knowledge (although I really doubt that many teachers would know that implementation detail).

Hello @jpelay, existing adventures may have such keywords but the warningbox isn't shown to them, one might argue that it should!

You have to start editing the adventure for the warningbox to be shown.

@hasan-sh
Copy link
Collaborator

I assumed that people would want to include the keywords as they are use in real programs, to_list is not really how you write that keyword

I think they will, especially after they see the message you provide in the warningbox!

You have to start editing the adventure for the warningbox to be shown.

Do you think we should check and show it at the beginning as well? If so, we could show it after the CKEditor is initialilzed.

@jpelay
Copy link
Member Author

jpelay commented Apr 1, 2024

Do you think we should check and show it at the beginning as well? If so, we could show it after the CKEditor is initialilzed.

Done, mate!

@jpelay jpelay requested a review from hasan-sh April 8, 2024 23:13
Copy link
Collaborator

@hasan-sh hasan-sh left a comment

Choose a reason for hiding this comment

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

We just need to fix the tests and we should be good to go:))) with tiny comments!

Comment on lines 12 to 13
cy.get('[data-cke-tooltip-text="Bold (Ctrl+B)"]').click()
cy.get('[data-cke-tooltip-text="Bold (Ctrl+B)"]').click()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason for these?! Or just to test them?!

Copy link
Collaborator

Choose a reason for hiding this comment

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

you could also control it from the instance we set on window. We use it here for instance: https://github.com/hedyorg/hedy/blob/main/tests/cypress/e2e/for-teacher_page/customize-adventure_page/adventure_content_field.cy.js

Copy link
Member Author

Choose a reason for hiding this comment

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

Any reason for these?! Or just to test them?!

Targeting the editor window to actually type on it is very hard with Cypress, so I just make it the focused element by clicking on one of the options and then use realType from cypress-real-events to actually type on the editor. Got that from this issue: cypress-io/cypress#26155

Copy link
Collaborator

Choose a reason for hiding this comment

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

you could also control it from the instance we set on window. We use it here for instance: https://github.com/hedyorg/hedy/blob/main/tests/cypress/e2e/for-teacher_page/customize-adventure_page/adventure_content_field.cy.js

This is working, isn't it?!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we could suggest our way:-)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is working, isn't it?!

It isn't, hence why I did it that way. Just setting the value of the editor doesn't fire a change event.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it does. But you need to give it an html tag that you mean to write; ckEditor.setData("<code>to</code>")`

Copy link
Collaborator

@hasan-sh hasan-sh Apr 11, 2024

Choose a reason for hiding this comment

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

This is probably more stable since you may remove the bold button from your bar or we add something else and ckEditor changes the attribute. I like this discussion and just posted a comment :))
cypress-io/cypress#26155 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes! You're right, updated the tests to reflect your suggestion, thanks!

Copy link
Collaborator

@hasan-sh hasan-sh left a comment

Choose a reason for hiding this comment

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

Thanks for all the work!

Copy link
Contributor

mergify bot commented Apr 14, 2024

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

Copy link
Contributor

mergify bot commented Apr 16, 2024

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 9679d15 into main Apr 16, 2024
12 checks passed
@mergify mergify bot deleted the suggestion_keywords branch April 16, 2024 23:51
Copy link
Contributor

mergify bot commented Apr 16, 2024

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

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

Successfully merging this pull request may close these issues.

3 participants