-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Feature/49060 group agenda items with sections #15291
Feature/49060 group agenda items with sections #15291
Conversation
… and drop support yet
…ng of code required
frontend/src/stimulus/controllers/dynamic/meeting-section-form.controller.ts
Outdated
Show resolved
Hide resolved
modules/meeting/app/contracts/meeting_sections/update_contract.rb
Outdated
Show resolved
Hide resolved
@@ -86,42 +86,89 @@ def update_sidebar_participants_form_component_via_turbo_stream(meeting: @meetin | |||
component: Meetings::Sidebar::ParticipantsFormComponent.new( | |||
meeting: | |||
), | |||
status: :bad_request | |||
status: :bad_request # TODO: why bad_request? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to ensure a non-ok HTTP response so that the modal doesn't auto-close after the form submission. Do you have a specific other status in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would make sense to me if there's a form which should show from validations - but in this case I don't see that. Additionally, having a static "bad_request" - even for successful submissions - seems like a misuse of the status.
However, I just saw that this method (update_sidebar_participants_form_component_via_turbo_stream
) is not used at all. Only update_sidebar_participants_component_via_turbo_stream
is called within the controller.
I would therefore remove update_sidebar_participants_form_component_via_turbo_stream
. @oliverguenther Or do I miss something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think it can be removed, it's a remnant from before the time we rendered the dialog/form separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for specs and general cleanup, looking very good already. The way you solved the first section feels very native. Let's try to get this done before we switch all our efforts to open points 👍
…n the section controller in order to get a homogenous behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to merge once the first implicit section is removed again as the spec correctly remarks 👍
https://community.openproject.org/work_packages/49060