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

Show obs - convert turbo-frame tags to div #2038

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

nimmolo
Copy link
Contributor

@nimmolo nimmolo commented Mar 18, 2024

Lower-maintenance solution for turbo updates on the show obs page:

We don't have to use <turbo-frame> tags for "surgical" updates to the DOM. <div> with an id is enough.

Switching to <div>s also means we don't need any special data attributes on nested <a> links. That's because links within a turbo-frame expect a response that replaces the turbo-frame they are nested within, and not a new page. (#2036) It seems like a gotcha that will confuse future developers.

Note: there's a separate existing issue here, and I don't know how or if it's related: #2039
Update: it wasn't related, but this PR fixes the issue. The obs id wasn't getting passed to the re-rendered new HR/CN links, so the links didn't work.

@coveralls
Copy link
Collaborator

coveralls commented Mar 18, 2024

Coverage Status

coverage: 94.428% (+0.003%) from 94.425%
when pulling 2f0120a on nimmo-convert-turbo-frames-to-divs-show-obs
into 155f008 on main.

@nimmolo nimmolo requested a review from mo-nathan March 18, 2024 03:21
@nimmolo
Copy link
Contributor Author

nimmolo commented Mar 18, 2024

@mo-nathan Should I maybe merge this, and sort out the existing delete/add refresh issue in a separate PR?

@mo-nathan
Copy link
Member

Looks good to me. I was not able to reproduce the behavior reported in #2039.

@nimmolo nimmolo changed the title Convert turbo-frame tags to div Show obs - convert turbo-frame tags to div Mar 18, 2024
@nimmolo nimmolo marked this pull request as ready for review March 18, 2024 23:41
@nimmolo nimmolo merged commit 065b407 into main Mar 19, 2024
5 checks passed
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.

None yet

3 participants