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

Switch empty state when public flag is toggled #16036

Merged
merged 10 commits into from
Jul 9, 2024

Conversation

klaustopher
Copy link
Contributor

One more left over ticket from #15971: When toggling the public state of the project list, re-render the modal

@klaustopher
Copy link
Contributor Author

I am very very confused why this does not work. It renders the exact same response body as other replace calls, but it is not replacing

Response that works:

<turbo-stream action="replace" target="share_modal_body">
    <template>
      <turbo-frame id="share_modal_body">...</turbo-frame>
    </template>
</turbo-stream>

Response that does not work:

<turbo-stream action="replace" target="share_modal_body">
    <template>
      <turbo-frame id="share_modal_body">...</turbo-frame>
    </template>
</turbo-stream>

If anybody has an idea, I'd be very happy to understand why

@klaustopher
Copy link
Contributor Author

It might boil down to how primer components mke the request. They generate a fetch request from their JS code.

https://github.com/primer/view_components/blob/ae8dbe23040ded74fab853ac3dc2068711c625f1/app/components/primer/alpha/toggle_switch.ts#L161-L172

@oliverguenther pointed out that the Accept header needs to contain text/vnd.turbo-stream.html but it doesn't since it just sets Accept: */*

@klaustopher
Copy link
Contributor Author

We have to wait for a decision on primer/view_components#2940 if we can use the component or if we need to rebuild a bit from scratch so we can use turbo with it. We can already merge this and create anotehr follow up in the future

@query.where("project_id", "=", entity.project.id)
end

@query.order(name: :asc) unless params[:sortBy]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe check for params is not needed as calls to order are appended

Copy link
Contributor Author

@klaustopher klaustopher Jul 9, 2024

Choose a reason for hiding this comment

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

The default order for a member is created_at, so it makes sense to reorder here depending on params being present

spec/controllers/projects/queries_controller_spec.rb Outdated Show resolved Hide resolved
@klaustopher klaustopher merged commit e61ce5c into release/14.3 Jul 9, 2024
11 checks passed
@klaustopher klaustopher deleted the switch-empty-state branch July 9, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants