Enable a modal to edit bookmark categories #1799
Conversation
|
|
||
| if BookmarkCategory.objects.filter(user=self.instance.user, name=name).exists(): | ||
| raise forms.ValidationError("This name already exists for a bookmark category") |
There was a problem hiding this comment.
This error message could be improved. Maybe "You have already created a bookmark category with that name"
|
|
||
| if request.method == "POST": | ||
| edit_form = BookmarkCategoryForm(request.POST, instance=category) | ||
| print(edit_form.is_bound) |
| category.save() | ||
| return JsonResponse({"success":True}) | ||
| if not edit_form.is_valid(): | ||
| print(edit_form.errors.as_json()) |
There was a problem hiding this comment.
😲 😲
I guess this whole if clause can be removed
There was a problem hiding this comment.
I guess you're referring to the second if block that just prints the form errors? I probably forgot to erase that since I think it was for debugging purposes. The first if statement I am quite sure it is required in order to update the category name in the database.
| @@ -195,7 +198,7 @@ const handleGenericModal = (fetchContentUrl, onLoadedCallback, onClosedCallback, | |||
| }; | |||
|
|
|||
|
|
|||
| const handleGenericModalWithForm = (fetchContentUrl, onLoadedCallback, onClosedCallback, onFormSubmissionSucceeded, onFormSubmissionError, doRequestAsync, showLoadingToast, modalActivationParam) => { | |||
| const handleGenericModalWithForm = (fetchContentUrl, onLoadedCallback, onClosedCallback, onFormSubmissionSucceeded, onFormSubmissionError, doRequestAsync, showLoadingToast, modalActivationParam, dataReloadOnSuccess) => { | |||
There was a problem hiding this comment.
dataReloadOnSuccess should probably be called triggerPageReloadOnSuccess for making it fully self-explainable
| handleGenericModal(modalUrl, undefined, undefined, true, true, modalActivationParam); | ||
| } | ||
|
|
||
| const handleDefaultModalWithForm = (modalUrl, modalActivationParam) => { | ||
| handleGenericModalWithForm(modalUrl, undefined, undefined, (req) => {showToast('Form submitted succesfully!')}, undefined, true, true, modalActivationParam, true); |
There was a problem hiding this comment.
You are enabling the reload on success by default, and also not providing a way to disable it unless handleGenericModalWithForm is used directly instead of an automatic binding. You should probably add a parameter to handleDefaultModalWithForm (e.g. reloadOnSuccess) which defaults to false, and then in the bind function, see if the element has a specific dataset property to decide if reloadOnSuccess should be true or false (e.g. data-reload-on-success)
There was a problem hiding this comment.
I am not sure that I fully understood your suggestion because to my understanding you do not need to check anything in the bind function if the parameter is sent properly in handeDefaultModalWithForm. By now my proposal would be to:
- set triggerPageReloadSucces to false in handleGenericModalWithForm
- add "element" as a parameter in handeDefaultModalWithForm
- send element.reloadOnSuccess to handleGenericModalWithForm or false as a default value if it is not specified.
- define a data-reload-on-success="true" parameter in the html definition of the toggle
There are other implemented changes you proposed in another branch (as in messageOnSuccess). Anyway, the result seems to work fine and would allow not to reload the page by default (only when specified in the element). Did I understand it correctly?
There was a problem hiding this comment.
Ok, it might be a good idea to simply pass element (also when binding), you can do it like that
Issue(s)
#1577
Description
addition of a basic modal to rename bookmark categories
Deployment steps: