-
Notifications
You must be signed in to change notification settings - Fork 295
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
💻 Add Ability to Rename Class #5978
Conversation
Wow thanks for the work and the very clear PR @Sidd-T!! We will take a look in our meeting tomorrow, if not before. Feel free to join the meeting to if you want to know the project better (see Discord for the link). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Sidd-T! Thanks a lot for your first contribution, we really appretiate it. However, I'd like to suggest a few changes before we are ready to ship this to teachers.
templates/customize-class.html
Outdated
<h2>{{_('customize_class')}}: {{class_info.name}}</h2> | ||
<div class="flex flex-row gap-4 items-center"> | ||
<h2>{{_('customize_class')}}:</h2> | ||
<h2 class="mt-0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of making the name of the class an input field, I would like if instead we put a pencil on the side of ne name, like
<h2 class="mt-0"> | |
<h2>{{_('customize_class')}}: {{class_info.name}} <i class="fa-solid fa-pencil"></i></h2> |
And when you click on it, you are greeted with a modal or the h2 turns into an input field, whatever works best for you! Right now, the behavior that I found was that after editing the field, a modals shows up, and then there's an error, I think we should keep one behavior or the other, but not both. See the attached video
2024-12-01.12-04-35.mp4
If you need to show the modal when clicking a button you can use this function: prompt
from modal.ts
And you can see an example of this being used in the function create_class
in the file teachers.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So uppon further review, I saw that rename_class
already uses the modal.prompt
, so instead of hooking it up to the change event of the input
just call it when someone press the pencil button I suggested earlier. And instead of pasing it this.value
you are going to use a value like: "What's the new name of this class" or something of the sorts, then I'll deal with making that value translatable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I think there's is something wrong with my environment, because I wasn't getting that modal showing up after the input edit, but I've changed it to be the pencil button now. It works for me but my environment may not be set up right, let me know if it works for you. For the prompt I just used the value you said here, as there is no message in the translations corresponding to rename class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sidd-T thank you so much for making the changes I suggested. I made a few more, mainly adding the translation and fixing a bug with the backend.
Congratulations on your first PR being merged, hope to see you around!
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). |
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). |
Fixes #5965 Added functionality to rename a class, going off the instructions in the issue comments with direct editing from change on the input element. I edited the exisiting rename_class function to directly send a request instead of opening a modal, as per the comment in the issue, but I believe it may be good to add a confirmation modal as well, let me know Also, I wasn't sure what to do with the rename_class in the teachers.ts file, which does not seemed to be used at all, so I left it unchanged **How to test** Follow these steps to verify this PR works as intended: * Log in as a teacher * create a class * in the customize class page, edit the input box, and click outside the box or press enter to confirm * verify the name has been changed by going back **Dev Testing** https://github.com/user-attachments/assets/422caab4-89d8-4d75-8a08-03899de1d5bb **Checklist** - [x] Contains one of the PR categories in the name - [x] Describes changes in the format above - [x] Links to an existing issue or discussion - [x] Has a "How to test" section Co-Authored-By: jpelay <[email protected]> Co-Authored-By: Jesús Pelay <[email protected]>
Fixes #5965
Added functionality to rename a class, going off the instructions in the issue comments with direct editing from change on the input element.
I edited the exisiting rename_class function to directly send a request instead of opening a modal, as per the comment in the issue, but I believe it may be good to add a confirmation modal as well, let me know
Also, I wasn't sure what to do with the rename_class in the teachers.ts file, which does not seemed to be used at all, so I left it unchanged
How to test
Follow these steps to verify this PR works as intended:
Dev Testing
https://github.com/user-attachments/assets/422caab4-89d8-4d75-8a08-03899de1d5bb
Checklist