[Orders Page] Fix Confirmation modals do not auto-close #13004
Conversation
- Execute the close method only when the current context modal is opened
| // Only execute close if there is an open modal | ||
| if (!document.querySelector("body").classList.contains('modal-open')) return; | ||
| // Only execute close if the current modal is open | ||
| if (!this.modalTarget.classList.contains('in')) return; |
There was a problem hiding this comment.
This close function is executed whenever the modal:close event is triggered.
On the Orders page, we have 3 confirmation modals e.g.
- Resend Confirmation
- Send Invoice Confirmation
- Cancel Order Confirmation
Consider the scenario where the Cancel Order Confirmation Modal is confirmed, then the modal:close event triggers. This close function gets triggered 3 times for each confirmation modal in sequential order (Resend Confirmation Modal -> Send Invoice Confirmation -> Cancel Order Confirmation) as each is listening for this event.
On the first close execution (for Resend Confirmation Modal), the modal-open class is removed from the body as per Line#20.
So, on further executions (for Send Invoice Confirmation and Cancel Order Confirmation), the existing guard condition would always be true, causing the method to return without any close operation. Further to this point, no other confirmation modal will be closed on the page.
So, updating the guard condition such that the close function only triggers if the current modal is opened.
I hope this explanation makes sense, please let me know if I'm missing something.
There was a problem hiding this comment.
Great, it looks like a good change, because it's working more directly on the element associated with the controller, instead of accessing the global context which is generally a bad idea.
| @@ -36,6 +37,7 @@ export default class extends Flatpickr { | |||
| sv: sv, | |||
| tr: tr, | |||
| en: en, | |||
| hu: hu, | |||
There was a problem hiding this comment.
Fixes the flatpickr invalid locale hu error as mentioned in the issue
There was a problem hiding this comment.
Good work.
I think this would be worth mentioning on this page: https://github.com/openfoodfoundation/openfoodnetwork/wiki/Internationalisation-%28i18n%29
It would be a helpful tip for someone when they're adding a new language.
chahmedejaz
changed the title
chahmedejaz
added
bug-s4
There was a problem hiding this comment.
Thank you!
The missing locale will probably happen again with new instances. But we don't want to add too many locales to the JS either. It increases the size and other instances don't need it. It would be great to have a more flexible solution where we set the locale on demand and load it from a CDN.
| // Only execute close if there is an open modal | ||
| if (!document.querySelector("body").classList.contains('modal-open')) return; | ||
| // Only execute close if the current modal is open | ||
| if (!this.modalTarget.classList.contains('in')) return; |
There was a problem hiding this comment.
Great, it looks like a good change, because it's working more directly on the element associated with the controller, instead of accessing the global context which is generally a bad idea.
| @@ -36,6 +37,7 @@ export default class extends Flatpickr { | |||
| sv: sv, | |||
| tr: tr, | |||
| en: en, | |||
| hu: hu, | |||
There was a problem hiding this comment.
Good work.
I think this would be worth mentioning on this page: https://github.com/openfoodfoundation/openfoodnetwork/wiki/Internationalisation-%28i18n%29
It would be a helpful tip for someone when they're adding a new language.
Yes Maikel, I strongly agree with this. Maybe in the future, we could incorporate it. |
Sure, David, let me add that. Thanks. |
|
David, It's added in the documentation here now. |
What? Why?
flatpickr invalid locale hu erroras mentioned in the issueWhat should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):