-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
[BUU] Remove Stimulus Reflex from Products screen #12328
[BUU] Remove Stimulus Reflex from Products screen #12328
Conversation
1fcd7c7
to
65efb8c
Compare
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.
I did only a quick review but didn't find anything obviously wrong. It looks easier than I thought to remove SR from the page.
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.
Looks fine !
5c7ff95
to
eebb25f
Compare
Sorry @filipefurtad0 I seem to have missed something in the rebase, I'll have to fix it up for testing tomorrow instead. |
d72eb78
to
4b5f169
Compare
@filipefurtad0 I've fixed the rebase, and added testing notes. Please test away! 🧪 |
We've found that we just can't rely in StimulusReflex (and the underlying WebSockets stack) to guarantee a response to a request. Because of this, there was intermittent issues when the server was overloaded with large requests, and the response never arrived, leaving an infinite loader, and a poor user wondering if anything was still happening.
Now there's a popup asking to confirm, which I think is worth keeping!
But we have more work to do.
This has an auto submit and can potentially work with Turbo, like on the Orders screen.
Dunno why, but the product was appearing once for each variant.
Otherwise there could be over 200 listeners on a page.
Dunno why, but this recently started occuring for me in dev and test. Browser update?
Merges change from fb09a7f
4b5f169
to
1425d52
Compare
Hi @dacook , Unfortunately, I just lost all my notes, but the summary is - let's merge. I went through all the test you indicate (thank you 🙏) and spotted only a minor regression, when editing a product/variant, with any given filter:
Not related to this PR
Also, I've merged this branch on top of the specs for BUU and spotted no additional failing specs. Merging! |
Hi Filipe, thanks for testing this one!
Good spotting, noted. I've fixed this in my next branch.
I thought it would be a good opportunity to test out the general usability of the page ;)
I can see how this is a bit annoying, but I found it usable: I was able to click "discard changes" to reset, or press CTRL/CMD-Z to undo. Or just type it in again ;) However there is a problem with units when creating a new variant.
🎉 |
I forgot to do this in openfoodfoundation#12328 [BUU] Remove Stimulus Reflex from Products screen
I forgot to do this in openfoodfoundation#12328 [BUU] Remove Stimulus Reflex from Products screen
I forgot to do this in openfoodfoundation#12328 [BUU] Remove Stimulus Reflex from Products screen
What? Why?
After much discussion, I have removed most of the StimulusReflex from the new Products screen. The plan is to make it more dynamic with Turbo/AJAX.
SR is still there for deleting records, which could still suffer the loading issues we had. (It's also used for the Edit Image modal, but this shouldn't have the loading issue).
I expect to continue with removing those, but it would be more efficient to transfer them directly to a Turbo/AJAX implementation later.
What should we test?
All the things!! 🫠 (almost)
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates