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

Fix bug where Idiomorph sometimes ignores data-turbo-permanent #1321

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

botandrose
Copy link
Contributor

@botandrose botandrose commented Sep 26, 2024

Hi folks, this is a redux of #1308 with an alternative strategy: fixing the issue in Idiomorph.

Motivating use-case

I'm building a collaborative issue tracking app, and one of the issue tracker app's primary use-cases has exposed a bug in the integration between Idiomorph and the data-turbo-permanent attribute. Each ticket contains a data-turbo-permanent checkbox to keep track of the client-side state of whether or not the ticket is currently expanded for that client or not. We also allow tickets to be reordered via drag-and-drop. The issue is that this data-turbo-permanent checkbox is not always preserved across morphs involving ticket reorders. There are other more serious ramifications of this bug (data loss!), but this is the simplest case to illustrate the issue.

Diagnosis and resolution

I have reduced this scenario down to the failing test case in this PR, and identified the issue as being within Idiomorph, which I've opened a PR for here: bigskysoftware/idiomorph#61 . However, much smaller "no-brainer" PRs from other Turbo contributors have gone ignored for months, so I'm not optimistic at its odds of a timely merge and release. Perhaps Turbo should maintain its own Idiomorph branch? At the moment, this PR is pointing Idiomorph to my own botandrose Idiomorph branch, which I'm sure is not ideal.

Thoughts?

@botandrose botandrose force-pushed the morph-reorder-with-data-permanent-children-redux branch from a363174 to 49f4504 Compare September 26, 2024 06:17
@botandrose botandrose force-pushed the morph-reorder-with-data-permanent-children-redux branch 5 times, most recently from b0dbea1 to 78f6e45 Compare December 24, 2024 21:46
@brunoprietog
Copy link
Collaborator

I see bigskysoftware/idiomorph#72 was finally merged. Would incorporating the use of twoPass help here?

@botandrose
Copy link
Contributor Author

botandrose commented Jan 8, 2025

Hi @brunoprietog, yes, but not quite yet! There's one outstanding bug in v0.4.0 that breaks Turbo data-turbo-permanent: bigskysoftware/idiomorph#83 . Once that gets merged and released, upgrading Idiomorph will become possible for a much-improved morphing experience in Turbo. I'm hoping we'll get a v0.4.1 or a v0.5.0 out the door very soon. Just gotta wait for @1cg to carve out the time to review and release.

@1cg
Copy link

1cg commented Jan 8, 2025

I am going to review the PR tomorrow and will update this issue

@brunoprietog
Copy link
Collaborator

Thanks!

@botandrose botandrose force-pushed the morph-reorder-with-data-permanent-children-redux branch 2 times, most recently from 9bce6eb to 794de7c Compare February 13, 2025 20:52
@botandrose
Copy link
Contributor Author

botandrose commented Feb 13, 2025

@brunoprietog After months of work, we've just released Idiomorph v0.7.1 which is a big leap forward in many many things, including fixing this motivating bug. Morphing should be much, much more correct now across the board.

So I've force-pushed this PR with three commits:

  1. A small bugfix to the Turbo->Idiomorph integration
  2. The motivating failing test
  3. The bump to Idiomorph v0.7.1 that fixes it.

Lastly, note that there is one additional feature in v0.7.1 that may be of interest here. Active focus/selection restoration after morph. I know Turbo already does this for non-morphing renders, but now the morphing render does it too via Idiomorph. If you want to evaluate that as a separate addition, it can be disabled by passing restoreFocus: false to Idiomorph.morph.

Finally, Carson has designated me as the lead of Idiomorph, so moving forward, I'll be happy to work with Turbo on any morphing-related issues or desires.

P.S. CI is red right now on main, so we're getting false failures on this PR. I'll rebase after #1368 or something similiar is merged.

@botandrose botandrose force-pushed the morph-reorder-with-data-permanent-children-redux branch from 9bce6eb to a7b9250 Compare February 14, 2025 14:18
@botandrose
Copy link
Contributor Author

And, we're green! Ready for review.

@jorgemanrubia
Copy link
Member

jorgemanrubia commented Feb 14, 2025

The new version looks fantastic. Let us test it a bit with our apps and I'll merge. Thank you so much for the amazing work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants