Skip to content
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

[Admin] Open edit and new forms in dialog with turbo frame #6046

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Dec 20, 2024

Summary

Opens new and edit forms of Admin resources in dialogs with Turbo Frames.
Removing the need of rendering the index table again on the form components.

Also handles the layout toggle for all turbo frame requests.

Includes the ground work from #6045

  • Shipping Categories
  • Adjustment reasons
  • Return reasons
  • Store credit reasons
  • Refund reasons
  • Tax categories
  • User roles
  • User store credit
  • Edit Stock items

Closes #5944

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

@tvdeyen tvdeyen force-pushed the open-modal-content-remotely branch 2 times, most recently from 22f5345 to 5e2c4da Compare December 20, 2024 10:57
@tvdeyen tvdeyen force-pushed the open-modal-content-remotely branch from 5e2c4da to 909607d Compare December 20, 2024 20:02
@tvdeyen tvdeyen changed the title [Admin] Open Shipping Categories edit and new forms in dialog with turbo frame [Admin] Open edit and new forms in dialog with turbo frame Dec 20, 2024
@tvdeyen tvdeyen marked this pull request as ready for review December 20, 2024 20:05
@tvdeyen tvdeyen requested a review from a team as a code owner December 20, 2024 20:05
@tvdeyen tvdeyen force-pushed the open-modal-content-remotely branch 2 times, most recently from 354c7c2 to a1e9278 Compare December 20, 2024 20:28
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 94.92754% with 7 lines in your changes missing coverage. Please review.

Project coverage is 89.59%. Comparing base (85a0a25) to head (bc0f636).

Files with missing lines Patch % Lines
...ontrollers/solidus_admin/stock_items_controller.rb 50.00% 2 Missing ⚠️
...rollers/solidus_admin/tax_categories_controller.rb 50.00% 2 Missing ⚠️
...mponents/solidus_admin/resources/base_component.rb 88.88% 1 Missing ⚠️
...nts/solidus_admin/tax_categories/edit/component.rb 0.00% 1 Missing ⚠️
...rollers/solidus_admin/return_reasons_controller.rb 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6046      +/-   ##
==========================================
- Coverage   92.52%   89.59%   -2.93%     
==========================================
  Files         385      794     +409     
  Lines        8400    18262    +9862     
==========================================
+ Hits         7772    16362    +8590     
- Misses        628     1900    +1272     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tvdeyen tvdeyen force-pushed the open-modal-content-remotely branch 2 times, most recently from 162d93c to ca8ae78 Compare December 20, 2024 21:07
@chaimann
Copy link

@tvdeyen i've checked out your branch and here's the screen recording of the "issues" i mentioned. You can see when I open the edit page in the new tab and click outside, it makes a request to edit url. Also when I click update the modal stays open.

However, I guess I understood it incorrectly what you meant about being able to visit the edit page. In my PR I have done it in a way that the underlying table is still rendered on edit page which I thought was your goal.
So in case we want to have just a modal on the edit page and nothing else, not closing it on all occasions specified above may still be a desired behaviour since we are not rendering anything else other than a modal, so basically there's no need to close it 😄

Screen.Recording.2024-12-20.at.23.38.25.mov

Though there's one more thing I discovered, not related to visiting edit page, but opening a modal with "turbo-framed" link and clicking outside:

Screen.Recording.2024-12-20.at.23.50.35.mov

as you can see it makes a request to index which I'm sure is unnecessary in this circumstance. Probably has something to do with how the clicking outside is implemented, but I have not yet checked how it works so can't tell for sure

@tvdeyen tvdeyen force-pushed the open-modal-content-remotely branch 3 times, most recently from f566aa1 to fd5cbe3 Compare December 23, 2024 08:48
@tvdeyen
Copy link
Member Author

tvdeyen commented Dec 23, 2024

@chaimann Now, I understand what you meant. Thanks for the screen recording. This helps to understand the issue.

@tvdeyen i've checked out your branch and here's the screen recording of the "issues" i mentioned. You can see when I open the edit page in the new tab and click outside, it makes a request to edit url. Also when I click update the modal stays open.

However, I guess I understood it incorrectly what you meant about being able to visit the edit page. In my PR I have done it in a way that the underlying table is still rendered on edit page which I thought was your goal. So in case we want to have just a modal on the edit page and nothing else, not closing it on all occasions specified above may still be a desired behaviour since we are not rendering anything else other than a modal, so basically there's no need to close it 😄

Yeah, I think that we do not display the index table on the /new routes is preferable. But we should adopt the layout. Maybe simply remove the dialog when the request is not a turbo-frame request?

The same is true for the redirect after successful form submission.
I will add something that makes the redirect dependent from the kind of request.

Though there's one more thing I discovered, not related to visiting edit page, but opening a modal with "turbo-framed" link and clicking outside:

as you can see it makes a request to index which I'm sure is unnecessary in this circumstance. Probably has something to do with how the clicking outside is implemented, but I have not yet checked how it works so can't tell for sure

This is how @close_path is implemented. I can see if we can change that, but in the end it's something I can live with in this case.

@tvdeyen tvdeyen force-pushed the open-modal-content-remotely branch 5 times, most recently from 3995aa3 to 05dabbd Compare December 23, 2024 15:49
@tvdeyen tvdeyen marked this pull request as draft December 23, 2024 15:52
@tvdeyen tvdeyen force-pushed the open-modal-content-remotely branch 2 times, most recently from 97b06a3 to bc0f636 Compare January 9, 2025 14:53
@tvdeyen tvdeyen force-pushed the open-modal-content-remotely branch 2 times, most recently from c771be0 to 9f5d275 Compare January 10, 2025 18:03
@tvdeyen tvdeyen force-pushed the open-modal-content-remotely branch 3 times, most recently from e02f07f to 6fa7a4f Compare January 13, 2025 20:09
@tvdeyen tvdeyen force-pushed the open-modal-content-remotely branch from 6fa7a4f to fba7722 Compare January 15, 2025 16:34
If a table has no rowUrl defined we currently redirect to
root path. Which is not preferable. We should do nothing instead.
@tvdeyen tvdeyen force-pushed the open-modal-content-remotely branch 2 times, most recently from 34bd4db to 16066fd Compare January 15, 2025 21:21
tvdeyen and others added 2 commits January 15, 2025 22:50
We do not want to render the whole admin layout if the request
is a turbo frame request. We just want to render the component's
html in the minimal rails turbo frame layout, which still gives us the
option to yield something into the head for ie. cache control.
We don't want to deal with Turbo caching issues in the new admin.
The content a customer agent or product administrator should always be
the latest and most accurate version.

Prefetching is a performance nightmare for an admin interface like ours.

Co-authored-by: Eugene Chaikin <[email protected]>
@tvdeyen tvdeyen force-pushed the open-modal-content-remotely branch from 16066fd to 0f2f8a2 Compare January 15, 2025 21:50
tvdeyen and others added 11 commits January 16, 2025 18:40
This controller and components should be used for the Solidus
Admin RESTful resource views. It handles Turbo Frame requests
from the forms inside the modal dialog and redirects or rerenders
via turbo stream accordingly. That way we can use normal links
and form submits for our resource views without the need for
custom JS.
This tells turbo to reload the page if we redirect from
a turbo frame request. No need to respond with a turbo
refresh stream.
@tvdeyen tvdeyen force-pushed the open-modal-content-remotely branch from 0f2f8a2 to ab838cb Compare January 16, 2025 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Admin] Table component rows do not contain links
3 participants