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 modal content remotely #6044

Closed
wants to merge 6 commits into from

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Dec 19, 2024

Needs #6043

Summary

Part of #5944

This adds a new Stimulus controller and action to the admin pages index component.

Links that have the click->ui--pages--index#openModal data-action now open the
content returned by the Rails controller remotely inside the modal. This works by disabling
the layout while rendering the action from the controller and nesting the turbo-frame
inside of the dialog component.

This removes the necessity to re-render the pages index component on the new and edit actions and
with that it reduces DB queries and the need to maintain the url params across actions.

As first page I migrated the Shipping Category page. Other pages will follow in sub-sequent PRs

Overall this uses less code and is more intuitive by leveraging what Rails., Stimulus and Turbo provides
us with.

"Let's delete some Dead Code"

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.

This allows to run prettier in your editor
Adjust the syntax of all admin js files to
prettier config.
This controller provides a openModal action that we can use to load
the modal remotely instead of returning the whole index page with
the table rendered below the modal.

Currently no page uses this, but it can be implemented page by page.
This removes the dialog element from the dom after closing it.
Combined with the new pages index controller we can open and close
modals without leaving the page. Much faster and less round trips.
If a table has no rowUrl defined we currently redirect to
root path. Which is not preferable. We should do nothing instead.
This is actual links to open the new and edit forms in the modal dialog
remotely.
@tvdeyen tvdeyen requested a review from a team as a code owner December 19, 2024 20:02
@tvdeyen tvdeyen requested review from MadelineCollier and removed request for a team December 19, 2024 20:03
@tvdeyen
Copy link
Member Author

tvdeyen commented Dec 19, 2024

@MadelineCollier I finally cracked it. I think the code is pretty much self-explanatory. I would like us to migrate all existing admin index pages to follow this pattern and only use this going forward. We will be able to remove even more code from the components if we are done with the migration.

Merry Christmas 🎄

@tvdeyen tvdeyen requested a review from a team December 19, 2024 20:05
@chaimann
Copy link

@tvdeyen can you have a look at #6045, it's similar to this one, but uses data-turbo-frame prop on the link instead of click handler, perhaps it will be even easier to migrate to?

@tvdeyen
Copy link
Member Author

tvdeyen commented Dec 20, 2024

@tvdeyen can you have a look at #6045, it's similar to this one, but uses data-turbo-frame prop on the link instead of click handler, perhaps it will be even easier to migrate to?

@chaimann Wow! Nice. Will take a look.

@tvdeyen
Copy link
Member Author

tvdeyen commented Dec 20, 2024

Closing in favor of #6045

@tvdeyen tvdeyen closed this Dec 20, 2024
@tvdeyen
Copy link
Member Author

tvdeyen commented Dec 20, 2024

Will rework the Shipping Category changes on top of #6045 once this is merged

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.

2 participants