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

Convert existing Admin modals to Turbo frames #5688

Conversation

spaghetticode
Copy link
Member

Summary

After introducing the first modal form backed by Turbo frames and Turbo refresh with #5674, we can convert the other existing ones to the same pattern.

Some insights can be obtained also by taking a look at the already mentioned PR that introduced the functionality for the first time.

The stock item edit modal differs slightly from the established pattern, as the URL for the modal is managed via a Stimulus controller and some data attributes, rather than the usual A tag (all the stock item row is clickable, and TRs can only have TH and TD as descendants). In order to keep things simple, this PR adds the ability of the Stimulus controller to parse the URL and, if a _turbo_frame attribute is present, it's used for adding Turbo frame navigation capability. The parameter is then removed from the URL, so it cannot produce further side-effects.

The PR could be split for easing reviews, one PR for each modal conversion, but the changes are similar across the modals, so unless splitting is encouraged, I'd rather keep these changes all together.

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.

We can remove all this custom behavior since we want the modal to
be rendered async on top of the current page.
This way, the modal can be loaded async into the current page.

Please note that the order page rendering, which is discarded on
turbo-frame calls, needs to be moved to the bottom, as it includes
an empty turbo frame tag with same ID that would interfere with the
modal rendering.
The `respond_to` block is required to make Turbo frames and streams
work.

With this change, the order edit modal can be rendered async on top
of the calling page (order edit). Form submission, when successful,
triggers a turbo stream page refresh, updating contents (namely the
order email) while preserving the scroll position.
URLs for editing addresses are handled via turbo frames, enclosing
the modals inside their appropriate turbo frame tag addded on the
page.
Just like we did a few comments before with the order email modal.
`Turbo.visit` already supports turbo frames, so we just need a way
to pass the information from the table HTML to the `visit` call.

This implementation, although arbitrary and possibly questionable,
has the merit of being simple, meaning that enabling turbo frames
in a table row requires just to update the URL to include the
`_turbo_frame` param. This param is removed from the URL, so it
won't have any possible further side effect in the application.
@spaghetticode spaghetticode force-pushed the spaghetticode/convert-modals-to-turbo-frames branch from b3b0e2c to 9d872ee Compare March 11, 2024 09:51
TR elements can have only TH and TD descendants, so we made table
rows linkable via Stimulus. Since A tags cannot be used for making
the row linkable, we can't leverage the default Turbo frame
functionality provided via the `data-turbo-frame` attribute.

For this reason, a workaround has been implemented with the previous
commit.
Since the modal open and closes on the current page, close links are
not needed anymore. Same goes for the explicit q and page params.

The `Cancel` button is simplified to use dialog standards.
Separated from the previous commit for easing reviews.
With the modal managed via turbo frame and turbo stream refresh,
all this custom explicit params management is not needed anymore.
@spaghetticode spaghetticode force-pushed the spaghetticode/convert-modals-to-turbo-frames branch from 9d872ee to 8de68cb Compare March 11, 2024 10:22
@spaghetticode spaghetticode marked this pull request as ready for review March 11, 2024 10:42
@spaghetticode spaghetticode requested a review from a team as a code owner March 11, 2024 10:42
Copy link
Contributor

@rainerdema rainerdema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@spaghetticode spaghetticode merged commit 79f3754 into solidusio:main Mar 13, 2024
12 checks passed
@spaghetticode spaghetticode deleted the spaghetticode/convert-modals-to-turbo-frames branch March 13, 2024 13:15
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