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

Project index turbofy #16246

Merged
merged 40 commits into from
Aug 23, 2024
Merged

Project index turbofy #16246

merged 40 commits into from
Aug 23, 2024

Conversation

klaustopher
Copy link
Contributor

@klaustopher klaustopher commented Jul 25, 2024

Make the project index view be able to render different components with turbo streams.

This is already functional. The Page header, menu, filter list, flashes and project list are already dynamically updated via Turbo Streams.


Closes https://community.openproject.org/work_packages/56557

@klaustopher klaustopher force-pushed the project-index-turbofy branch 5 times, most recently from 9824cd2 to 5e538dd Compare July 29, 2024 14:20
@klaustopher klaustopher marked this pull request as ready for review July 30, 2024 08:48
@klaustopher
Copy link
Contributor Author

klaustopher commented Jul 30, 2024

Handing over the baton to you, @aaron-contreras.

This PR is mostly done in its core functionality. I have wrapped the page header and the table in turbo wrappers that we can replace from the backend. The very advantage that we have is that all modifying actions on a ProjectQuery redirect back to the ProjectsController#index action. So we do not need to define any special replacing behavior in those controller actions, but we can always rely on the redirect and then in the #index action, we can replace the relevant parts.

With the help of turbo_power we can even do the pushState to modify the history and change the source of the <turbo-frame> of the turbo frame to reload the menu.

I have also wrapped the flash element in a <turbo-frame> so we can replace it via stream. Oliver and Mir are working on another way to do this, by having a special action in #16284 that can render a primer banner. Maybe this makes more sense to use here as well.

The next step would be to change the filter component to also issue the request via turbo stream when a filter is changed, get rid of the apply button and basically allow setting filters like we do on the work package page. That should then be used to implement the new always visible text filter.


There's also some strange behavior in the tests right now. When manually trying in a browser, the toast flash messages do show up. In the tests it does not work, that's why the tests on toasts are still commented out

This makes sure that the DOM can remain agnostic of this behavior
of auto-submitting and we can rely on the Stimulus controller
taking care of most of this logic which in the end, pretty much
relies on that end of the spectrum.
Copy link
Contributor

@dombesz dombesz left a comment

Choose a reason for hiding this comment

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

Overall looks good, just a few questions @aaron-contreras 🌴

spec/helpers/sort_helper_spec.rb Outdated Show resolved Hide resolved
spec/features/projects/persisted_lists_spec.rb Outdated Show resolved Hide resolved
app/controllers/projects_controller.rb Show resolved Hide resolved
@@ -169,7 +174,8 @@

# Can save the project list
projects_index_page.save_query
projects_index_page.expect_toast(message: "The modified list has been saved")
# TODO: Toast is currently not rendered in turbo actions
# projects_index_page.expect_toast(message: "The modified list has been saved")
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor

@aaron-contreras aaron-contreras Aug 21, 2024

Choose a reason for hiding this comment

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

Do you have any idea why the toast is sent as a stream properly but isn't on the test suite e473b9c? I believe Klaus mentioned there was some funny behavior with the toast in the specs. There should be a PR Oliver and Mir are working on for a better way to deal with this that SHOULD resolve this weird behavior in specs. @dombesz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been a while ago, but I think there was something problematic because the way how the toast javascript was triggered was that it only worked on the page load and it did not properly work with turbo. But I'm not 100% sure where this was

@dombesz dombesz self-requested a review August 22, 2024 12:07
@dombesz dombesz merged commit 3012b28 into dev Aug 23, 2024
12 checks passed
@dombesz dombesz deleted the project-index-turbofy branch August 23, 2024 13:51
akabiru added a commit that referenced this pull request Sep 2, 2024
As of #16246 the project list is "turbofied" by default
akabiru added a commit that referenced this pull request Sep 2, 2024
As of #16246 the project list is "turbofied" by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants