-
-
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
Fix URL State management on the Products page #12678
Fix URL State management on the Products page #12678
Conversation
I'm occupying the au-staging for while, need to validate the fix on staging with real data. Thanks. |
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.
Great, it looks like it resolves the bug, but it removes the dynamic Turbo feature, which makes the page load seem slower :'(
It looks like Turbo has a feature to update the browser history, would this help?
https://turbo.hotwired.dev/handbook/frames#promoting-a-frame-navigation-to-a-page-visit
Could you try that?
- Fix the scenario when per_page is selected as 100 and next page is clicked, then per_page is empty in the request. - Expected behavior should be that it retains the per_page selected previously
b8e92e8
to
1af811c
Compare
Hi @dacook - Thanks for pointing that out. It's working great! 👍 |
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.
Yay! I had a little play with it and it all looks good to me.
Hey @chahmedejaz , This PR correctly introduces URL changes, when filters are used. This is demonstrated on the picture below (upper part, before staging; down part, after staging): However, it seems the page returns an endless spinner, if one uses the back option, on the browser. Steps to reproduce, the user:
Then, click the back option of your browser a few times; wait between each click - notice the page is never really loaded. I'll try this in another staging server, just to be sure this is not a data issue. |
I have my fingers crossed this is a data problem! it would explain something the CA instance has been experiencing... |
Unfortunately, I was able to reproduce the endless spinner in another staging server, after having several search filters and using the back option on the browser: I'm not sure what's better here: do you feel it's best to work on another branch, after merging @chahmedejaz ? I'm a bit concerned about the endless spinner, which will confuse users. So I think it could be a good idea to move this one back to In Dev. What do you think? |
I'll look into it @filipefurtad0. Thanks for raising this one. |
Ok, thanks @chahmedejaz. I had still another go at testing this, and pulled the branch into my local env.: I found the issue (endless spinner by clicking the browser back option) to be still reproducible as well, which I think removes the hypothesis of bad data (ping @RachL). Moved back to In Dev . |
Thanks @filipefurtad0 I'll look into this issue today. |
@@ -1,6 +1,6 @@ | |||
= form_with url: admin_products_path, id: "filters", method: :get, data: { "search-target": "form", 'turbo-frame': "_self" } do | |||
= form_with url: admin_products_path, id: "filters", method: :get, data: { "search-target": "form", 'turbo-frame': "_self", 'turbo-action': "advance" } do |
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.
Hi @dacook - Here are my findings on the issue raised by the @filipefurtad0:
- Due to
'turbo-frame': "_self"
on the above form submission, it targets the following frame, hence it gets thearia-busy="true"
attribute added when it's being updated.
%turbo-frame#products-content{ target: "_top", refresh: "morph" } - This causes the loader to show up as per our CSS rules
- The root cause here is that this same page (with
aria-busy="true"
) is then cached by Turbo, and upon navigation back, thearia-busy="true"
is still present on the (cached) page - I think this is a bug in Turbo, what do you think? It should sanitize the page so that no
aria-busy
is present on any frame. Or cache the page before making it busy.
For the solution, I had many, For example:
- remove
'turbo-frame': "_self", 'turbo-action': "advance"
from the affected links so that by our default rule, each page will be replaced by "_top" frame - This fixes our current loader problem, however, by this approach, we won't be getting the
aria-busy="true"
on the above-mentioned frame, so no loader upon UI button clicks.
Another solution is that I think we should opt for this one:
-
We should disable the Turbo cache because by default, we are also not caching pages in the browser, hence navigation back gets the page by the server.
-
I've implemented this here, please review: a6efad7
Please let me know what you think about it, if you are okay with this, we are good to move it back to testing. Thanks
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.
Hi Ahmed, this looks tricky. I'm sorry I'm not feeling well today and am not able to fully understand. A few thoughts:
I'm wondering what difference it makes if we remove the morph
method?
I'm hesitant to disable cache entirely in case it would be helpful in other circumstances. I'm wondering if choosing no-preview
would help (https://turbo.hotwired.dev/handbook/building#opting-out-of-caching)?
But If no-cache
is the only way to solve it, then go ahead.
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.
Hi Ahmed, this looks tricky. I'm sorry I'm not feeling well today and am not able to fully understand. A few thoughts:
I'm wondering what difference it makes if we remove the
morph
method?I'm hesitant to disable cache entirely in case it would be helpful in other circumstances. I'm wondering if choosing
no-preview
would help (https://turbo.hotwired.dev/handbook/building#opting-out-of-caching)? But Ifno-cache
is the only way to solve it, then go ahead.
No worries, David. Please take a rest and get well soon.
I've tried using no-preview
as cache control and removing morph as well, however, it's of no use. The issue explained here remains the same.
In the context of Turbo, Morphing is a way to handle page reloads/refreshes. When a page/frame refresh is set to morph, then Turbo doesn't fully replace the page/frame. It calculates the changes between the current HTML response and the current page/frame and only updates the changes in the current frame/page, rather than the full replacement.
Ref: https://turbo.hotwired.dev/handbook/page_refreshes#morphing
I'm afraid, disabling Turbo cache seems to be the only solution. 😕
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.
Thanks Ahmed, I guess that's the only option, please go ahead.
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.
Sorry forgot to approve.
I'm afraid this might limit performance enhancements later, but we will have to deal with that later.
Thanks, David. Sure, If we face any performance issues due to Turbo Cache, then we will see the way to deal with it. |
@filipefurtad0 - It's ready for revalidation. Please let me know if the issue still persists. Thanks. |
Thank you so much for your notice, to re-test this PR @chahmedejaz . It's looking great now: https://www.loom.com/share/65c93ea3ee9043eea50eff998b205237 I've marked it for release as introducing user-facing changes, as it enables the browsing possibility, throughout different filters. Awesome! Merging. |
7bcf320
into
openfoodfoundation:master
What? Why?
data-turbo-action = advance
to the affected form submissions such that the default form submission behavior is not affected as well as turbo remains intact.What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):