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

Crossword: remove hard-coded English string, and fix wrong unit test #732333 #31

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

BruceGoodGuy
Copy link
Contributor

Hi @timhunt,

I've made some necessary changes to address the ticket. I apologize for any mistakes I've made. Could you please help me review it?
Commit ID: b6fe016c6f5bea50e8a8772baafa4447943f4de9
Thank you very much.

@timhunt
Copy link
Member

timhunt commented Oct 27, 2023

Is this a crossword issue or a StudentQuiz issues? Did you copy-paste the wrong thing when making this pull request?

@BruceGoodGuy BruceGoodGuy changed the title StudentQuiz: There are some issues of Add completion criteria #732619 Crossword: remove hard-coded English string, and fix wrong unit test #732333 Oct 27, 2023
@BruceGoodGuy
Copy link
Contributor Author

Hi @timhunt
I apologize for my mistake, I just amended and pushed the new commit.

@timhunt
Copy link
Member

timhunt commented Oct 27, 2023

So from what I can see here, there are three fixes in one commit:

  1. Typo fix in the wrongoverlappingwords string: OK.
  2. Fix the unit test: OK.
  3. Try to fix the hard-coded string. Is there some reasone why you are doing your own re-implementation of getString in JavaScript? Surely you should use the standard Moodle function here?

@BruceGoodGuy
Copy link
Contributor Author

Hi @timhunt ,
Thank you for your feedback. Initially, I used the JavaScript get_string function in crossword_grid.js to obtain a string. However, it caused a delay issue in the first time that the user pressed the preview button. Please refer to the attached video:

evidence_cw.mp4

The delay is caused by JavaScript, which needs to make a server call to retrieve the language string using the get_string method. To resolve this problem, I had to implement another approach. This approach involves using the PHP method to fetch a placeholder language string, passing it to JavaScript, and then replacing the placeholder with the appropriate data.
I hope this explanation satisfies your concerns.
Please let me know your thoughts. Thank you so much.

@timhunt
Copy link
Member

timhunt commented Oct 30, 2023

So, fix the delay. Don't re-implement a huge chunk of standard Moodle functionality.

Probably the easiest way is: when the page first loads - so in your init method, call get_string so that the string we need is already cached.

(Note, recommended import is import {get_string as getString} from 'core/str'; so the JS code follows the coding style.)

@BruceGoodGuy
Copy link
Contributor Author

Hi @timhunt,
Thanks for your feedback, I have made a code changes based on your suggestion.
commit ID: 99745567d43df5b29f7bf1db25c8851c0e5bfe3e
Could you please help me to review it again? Thanks so much.

@timhunt timhunt merged commit daa77ae into moodleou:main Nov 1, 2023
5 checks passed
@timhunt
Copy link
Member

timhunt commented Nov 1, 2023

Thanks. Merged.

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

Successfully merging this pull request may close these issues.

2 participants