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

Add turbo: true as an option for ToggleSwitch #2964

Merged
merged 3 commits into from
Jul 26, 2024

Conversation

klaustopher
Copy link
Contributor

What are you trying to accomplish?

As described in #2940, we are using a ToggleSwitchForm and in our case the response is a turbo stream that should replace some other elements in the page. The current implementation does not allow for that as the fetch request does not trigger any behavior associated with Turbo. To do that we need to explicitly set the Accept header to allow text/vnd.turbo-stream.html and when the response comes back, we need to pass it to Turbo.renderStreamMessage to be rendered.

In #2940 we also discussed making the ToggleSwitch have a proper form, but while experimenting with this I found that this completely changes the existing behavior and I wanted this change to be as unobstrusive as it could be.

Integration

The default is turbo: false, so nothing changes in existing code.

List the issues that this change affects.

Closes #2940

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

The code does not change any default behavior. One has to intentionally activate turbo with turbo: true as a param to the ToggleSwitch to get the new behavior. Also the response needs to provide the correct content type to be forwarded into the turbo renderer.

What approach did you choose and why?

See above

Anything you want to highlight for special attention from reviewers?

There are currently no tests for the call to Turbo.renderStreamMessage, would be interested in any pointers how to add proper tests.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@klaustopher klaustopher requested a review from a team as a code owner July 23, 2024 10:02
Copy link

changeset-bot bot commented Jul 23, 2024

🦋 Changeset detected

Latest commit: b0a1e30

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@jonrohan jonrohan left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks

Copy link
Contributor

@camertron camertron left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@camertron
Copy link
Contributor

@klaustopher would you mind adding a changeset? Run npx changeset to get started.

@klaustopher
Copy link
Contributor Author

Will do tomorrow morning 👍

@klaustopher
Copy link
Contributor Author

Changeset is added

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.

ToggleSwitchForm does not work with Turbo Responses
3 participants