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

Refactor HTMX Implementation to avoid duplicate create forms #1119

Merged
merged 16 commits into from
Dec 3, 2024

Conversation

caseyhans
Copy link
Collaborator

@caseyhans caseyhans commented Oct 16, 2024

Refactors our HTMX views to avoid having multiple create forms at once, since this introduced bugginess.

The general pattern is this (refer to lit workflows as a good example):

  • In the template where you want the create form to exist, add a div with the classes create-row hidden
  • On the create button, add hx-target=".create-row" hx-swap="outerHTML"
  • In the edit form template, add the class create-row if this is a create and not an update (so that duplicate button presses will replace this item). Don't delete the form if a user hits cancel, just hide.

@caseyhans
Copy link
Collaborator Author

@shapiromatron ready for a draft review--still have UDF binding and mgmt task viewsets left

@caseyhans caseyhans marked this pull request as ready for review October 30, 2024 18:26
@caseyhans caseyhans requested a review from rabstejnek October 30, 2024 18:26
Copy link
Collaborator

@rabstejnek rabstejnek left a comment

Choose a reason for hiding this comment

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

There were a few bugs I witnessed when testing this out:

  • The cancel button doesn't work on label create forms in the HAWC Prime flavor. More specifically it looks like the hidden css is being overridden by flex css after conferring with Casey.

Other optional changes:

  • I noticed usage of classes for the create row htmx trigger. In practice this should only be a single element, so I'd personally prefer using an id, but it doesn't really make a difference with functionality. This change may be a bit branching so I'd be fine if it wasn't addressed.

Edited this comment after some bugs turned out to be local.

@caseyhans
Copy link
Collaborator Author

caseyhans commented Nov 15, 2024

@rabstejnek I can't recreate any of these bugs. The first two are css-related, so a hard refresh might fix it. Not sure about the last bug. I'll keep trying to replicate the bugs and reach out for a meeting otherwise. I'll also look into using IDs for the create rows, that's a good call

@caseyhans
Copy link
Collaborator Author

caseyhans commented Nov 15, 2024

OK the label cancel bug was some weird issue with the 'hidden' class being overwritten by 'd-flex' class with PRIME style. Good catch! Glad we're fixing this, not sure where else this bug might have been present...

@caseyhans
Copy link
Collaborator Author

caseyhans commented Nov 15, 2024

Ok, I tried swapping the create-row htmx target to IDs instead of classes, but it was introducing some weird bugginess to the autoscroll script that I couldn't pin down. Since it's not really a functional change anyways, I think we're OK to leave it as a class. If the change is worth the time, I can keep working on it.

@caseyhans caseyhans requested a review from rabstejnek November 15, 2024 21:39
@caseyhans caseyhans assigned rabstejnek and unassigned caseyhans Nov 15, 2024
@shapiromatron
Copy link
Owner

Ok, I tried swapping the create-row htmx target to IDs instead of classes, but it was introducing some weird bugginess to the autoscroll script that I couldn't pin down. Since it's not really a functional change anyways, I think we're OK to leave it as a class. If the change is worth the time, I can keep working on it.

Seems reasonable to me, I think this looks like it's ready for my review. Thanks yall!!

Copy link
Owner

@shapiromatron shapiromatron left a comment

Choose a reason for hiding this comment

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

Functionally, it works great, and fixes the bugs I introduced with my alternative approach. Since the selectors on pages with multiple creation forms use a dual selector, .foo.bar, I think this should work fine in practice. https://stackoverflow.com/a/3772305/906385

Nice job!

@shapiromatron shapiromatron merged commit c1c58ff into main Dec 3, 2024
6 checks passed
@shapiromatron shapiromatron deleted the refactor-htmx-create branch December 3, 2024 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants