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: "previous versions" button failing after transpose #26

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

Samuelodan
Copy link
Contributor

@Samuelodan Samuelodan commented Aug 10, 2024

this change stops the button from refreshing (or being replaced) and losing its event listener for opening the modal.

I found two other ways to fix this:

  1. wrapping the chord sheet content in a turbo frame and targeting that turbo frame from the "Previous versions" button.
  2. changing the transpose button to cause a full page reload. This one made the experience janky, so I didn't like it.

I'd like to hear your thoughts on the issue. I suspected the modal controller was getting disconnected after transposing, but that wasn't the case. The connection remained, but the button defaulted to sending a post request.

I also considered changing the button method to get, using a link, or even using a plain HTML button (no request will be sent) so it wouldn't cause the server error, but I don't know how useful that would be if the button's intended function weren't happening.

Lastly, I noticed the bug was a little different in prod. It looked like what you'd get if there were no matching turbo frame in the response. Still, I reckon that's better than the error in main.

this change stops the button from refreshing (or being replaced) and losing its event listener for opening the modal.
@Samuelodan
Copy link
Contributor Author

fixes #25

@Samuelodan Samuelodan changed the title fix: fix "previous versions" button failing after transpose fix: "previous versions" button failing after transpose Aug 10, 2024
@stufro
Copy link
Owner

stufro commented Aug 20, 2024

Thank you so much for fixing this! Looking at the options you described I think the one you picked is the best solution. I'll get this deployed asap. Thank youuuu :)

@stufro stufro merged commit bcad2dd into stufro:main Aug 20, 2024
@Samuelodan
Copy link
Contributor Author

Ah, I'm happy to hear you think so too. Thank you for the opportunity 😊

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.

2 participants