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

💻 Enhance customize-adventure page #5026

Merged
merged 22 commits into from
Feb 26, 2024
Merged

💻 Enhance customize-adventure page #5026

merged 22 commits into from
Feb 26, 2024

Conversation

hasan-sh
Copy link
Collaborator

@hasan-sh hasan-sh commented Jan 23, 2024

Fixes #4632

What?
There are two main features introduced in this PR.

  • show all classes that use the current adventure in one or more levels.
  • replace the plain text editor by a WYSIWYG editor CKEditor.

How to test?
go to the customize-adventure page and use the editor and see the classes (if any).

Notes
The editor is the main change, thus. See the following:

image

As can be seen, the toolbar has different plugins that teachers can utilize in order to enhance their experience with writing/creating custom adventures. For instance, teachers can make text bold by any of the obvious button in the toolbar. Alternatively, they could press CTRL-B to do so. Similarly, they can add ordered and unordered lists. Or create different headings for better visualization and attracting students' attention.

Perhaps we could add a small paragraph that explains such features. But we certainly need to explain that the code button makes a keyword, and the code block button creates a pre tag that would be rendered as a code that students can use. The pre tags aren't highlighted still, so further investigation could be done on how to do so. However, i don't think it's really needed since the editor really shows that it's a piece of code. And as Felienne noted that teachers can always preview what they have written so far, with syntax highlighting since that functionality is just HTML and would use codemirror.

@ghost
Copy link

ghost commented Jan 23, 2024

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@hasan-sh hasan-sh marked this pull request as ready for review February 13, 2024 11:48
@hasan-sh hasan-sh requested a review from jpelay February 13, 2024 11:49
Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

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

Hello Hasan! I think this is great, and looks amazing. I just had some comments that you can see in the comments bellow :D

Also, I think the default explanation looks a bit broken:

image

static/js/adventure.ts Outdated Show resolved Hide resolved

ClassicEditor
.create(editorContainer, {
codeBlock: {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe in the future we could investigate integrations between CKEditor and CodeMirror. I found this plugin (https://ckeditor.com/cke4/addon/codemirror), but is quite old, but it suggest that perhaps such a thing could be possible with newer versions of CodeMirror and CK

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it's quite old indeed! Let's see how it goes!

Copy link
Member

Choose a reason for hiding this comment

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

Do we need an issue for this so we don't forget?

static/js/adventure.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
import ClassicEditor from "@ckeditor/ckeditor5-build-classic";

export interface CustomWindow extends Window {
Copy link
Member

Choose a reason for hiding this comment

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

I think perhaps we could give this interface a more descriptive name, because it's not clear right now the purpose of it. If it's just meant to be used to hold the ckEditor, then perhaps CKWindow could be a fine option? What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it's meant to be used overall in TS. This due to the fact that TS will complain when we try to attach variables to the window object! So, custom window sounded logical to me. I don't mind renaming it if you have other suggestions! Maybe i could add a comment in that file to explain its purpose?

static_babel_content.json Outdated Show resolved Hide resolved
@hasan-sh
Copy link
Collaborator Author

Also, I think the default explanation looks a bit broken:

by default you should click on the exclamation mark, but I think it's better to show it then since it wasn't clear for you, how would it be for users /:

Copy link
Member

@Felienne Felienne left a comment

Choose a reason for hiding this comment

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

Loooove this new look @hasan-sh! And thanks for the review @jpelay!

Copy link
Member

@Felienne Felienne left a comment

Choose a reason for hiding this comment

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

Ow, we still need to fix the default explanation, sorry!

@hasan-sh
Copy link
Collaborator Author

Ow, we still need to fix the default explanation, sorry!

Updated! I still want to add the possiblity to create links, so wanna add an extra button to the toolbar for that!

@jpelay
Copy link
Member

jpelay commented Feb 19, 2024

Ow, we still need to fix the default explanation, sorry!

Updated! I still want to add the possiblity to create links, so wanna add an extra button to the toolbar for that!

Ping me when that's ready so I can approve the PR :D

@hasan-sh
Copy link
Collaborator Author

Ping me when that's ready so I can approve the PR :D

@jpelay I want to do that later, perhaps when we all use it and have further feedback:)

Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

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

Hi @hasan-sh! So before merging this, we need to improve the way that the default explanation is shown, for that we need to change the string of the label template_code to this:

msgid "template_code"
msgstr ""
"This is the explanation of my adventure!\n"
"\n"
"This way I can show a command: `{print}`\n"
"\n"
"But sometimes I might want to show a piece of code, like this:\n"
"```\n"
"{ask} What's your name?\n"
"{echo} so your name is \n"
"```"

And also, we need a way to convert the teacher adventure's to this, otherwise they're also shown in a funky way.

I have a further comment, and I don't know if this was if I add a code section, then I have no way to get out of the code section, look:

2024-02-20.11-32-23.mp4

@hasan-sh
Copy link
Collaborator Author

we need a way to convert the teacher adventure's to this, otherwise they're also shown in a funky way.

Yes that's very true!

I have a further comment, and I don't know if this was if I add a code section, then I have no way to get out of the code section, look:

If you just pressed 3 enters hahah that's the escape. I thought of adding that to the explanation as well. Also, i think i'll create an issue to explain this new editor in teacher manual.

Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

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

Great work!

Copy link
Contributor

mergify bot commented Feb 26, 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 8916130 into main Feb 26, 2024
11 checks passed
@mergify mergify bot deleted the adventure-enhancement branch February 26, 2024 20:08
mergify bot pushed a commit that referenced this pull request Mar 7, 2024
Fixes #5141 

**Key points**
- a quize/parson is added as a tab in the customize-class page. This makes it closer to how it'd look like for students.
- the current "hide quize/puzzle" switches will behave as "toggle all quizes/puzzles". Why? Consider the following:
  - the teacher decides to hide all quizes. 
  - they are removed from all levels,
  - they can of course still choose to show a quize in certain levels, while keeping the rest hidden.
  - however, what if they (for whatever reason) want to show all quizes but hide one or two?
  - this scenario is likely because until now the "hide quize" behaves as toggling all quizes, so it should still behave similarly.
  - thus it could be considered as resetting all quizes/puzzles.
- the order of puzzle and quiz tabs are always fixed to be always the last two tabs, respectively.

Something went missing when i added the key translations for #5026 (check 6ee2c4f#diff-852046eb47f474139974edf5e69ed9758f9fde82e1432c49d772b89aef354b37), so I'm adding them here.

**How to test?**

- go the customize-class page
- add/remove a quiz/puzzle
- reset all puzzles/quizes to test that functionality.
   - _note_ that if you disable all quizes and you add one quiz in level 2, for instance, the disable checkbox will be updated since not all quizes are disabled now. (it needs a refresh for now for that to be reflected in the DOM, but this should be dealt with in #4888)
- log in as a student or preview as teacher to see  your changes from the perspective of students.
- verify that everything works as expected.
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.

[UI idea] Adventure page enhancements
3 participants