[OP-19415] Convert FilterForm to ViewComponent - #23488
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the filters UI building block by replacing the former Filters::FilterForm (a Primer ApplicationForm) with Filters::FilterFormComponent (a ViewComponent), passing the form builder explicitly and rendering via a standard ERB template to reduce coupling to Primer internals.
Changes:
- Introduces
Filters::FilterFormComponentas anApplicationComponent, including a dedicated.html.erbtemplate, while keeping the existing filter-row composition viaPrimer::Forms::FormList. - Updates
Filter::FilterComponent, Lookbook previews/docs, and specs to instantiate and render the new component withbuilder:. - Adjusts related inline documentation/comments to reference the new component name.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| spec/components/filters/filter_form_component_spec.rb | Updates component spec to exercise Filters::FilterFormComponent and its builder keyword arg. |
| lookbook/previews/open_project/filter/filter_form_preview/with_hidden_input.html.erb | Switches preview rendering to Filters::FilterFormComponent.new(builder: ...). |
| lookbook/previews/open_project/filter/filter_form_preview/with_active_filter.html.erb | Switches preview rendering to the new component API. |
| lookbook/previews/open_project/filter/filter_form_preview/for_a_work_package_query.html.erb | Updates legacy Query preview comments and rendering to use the component. |
| lookbook/previews/open_project/filter/filter_form_preview/default.html.erb | Updates default preview to render the component with builder:. |
| lookbook/previews/open_project/filter/filter_form_preview/combined_with_other_inputs.html.erb | Updates composed FormList preview to include the component alongside forms. |
| lookbook/docs/patterns/11-filter-forms.md.erb | Renames and updates documentation from FilterForm to FilterFormComponent. |
| app/models/query.rb | Updates references in comments to the new component name. |
| app/models/queries/work_packages/filter/filter_for_wp_mixin.rb | Updates comment describing which UI dispatch renders these filters. |
| app/components/filters/filter_form_component.rb | Converts implementation from Primer form object to ApplicationComponent with explicit builder: and helper-based rendering. |
| app/components/filters/filter_form_component.html.erb | Adds ERB template implementing wrapper/hidden-input rendering logic. |
| app/components/filter/filter_component.rb | Updates filter_form to return Filters::FilterFormComponent with builder:. |
myabc
marked this pull request as ready for review
June 2, 2026 10:52
2 tasks
myabc
changed the title
myabc
force-pushed
the
code-maintenance/75446-filters-form-component-rework
branch
from
June 2, 2026 15:16
ed2cd0c to
f6700cc
Compare
Replaces `Filters::FilterForm` (an `ApplicationForm` subclass) with `Filters::FilterFormComponent` (an `ApplicationComponent`). The old form overrode `:nodoc:` Primer hooks (`before_render`, `perform_render`) and read semi-public ivars (`@builder`, `@view_context`). The new component receives the builder as an explicit keyword arg and uses a standard ERB template, reducing Primer internal coupling from five semi-public APIs to one (`FormList`). https://community.openproject.org/wp/OP-19415
Uses `ConditionalWrapper` to eliminate template duplication in the `wrap_with_controller` branch, and `merge_data` from `Primer::AttributesHelper` for Stimulus data attributes. https://community.openproject.org/wp/OP-19415
myabc
force-pushed
the
code-maintenance/75446-filters-form-component-rework
branch
from
June 2, 2026 17:01
f6700cc to
a9742d0
Compare
Stimulus supports multiple controllers on one element, but Primer's `merge_data` is not controller-aware and silently drops a caller's `data-controller` when a component merges in its own. Treat `controller` as a plural data attribute so the values concatenate instead.
Use OpPrimer::AttributesHelper so a caller-supplied data-controller is concatenated with the required filter--filters-form controller instead of being overwritten. The wrapper spec now asserts both controllers survive.
3 tasks
HDinger
approved these changes
Jun 3, 2026
HDinger
left a comment
HDinger
left a comment
Contributor
There was a problem hiding this comment.
The code changes look good to me. 👍 I am not 100% sure whether the CI failures might be related.
Comment on lines
+38
to
+45
| module AttributesHelper | ||
| include Primer::AttributesHelper | ||
|
|
||
| PLURAL_DATA_ATTRIBUTES = (Primer::AttributesHelper::PLURAL_DATA_ATTRIBUTES + %i[controller]).freeze | ||
|
|
||
| def merge_data(*hashes) | ||
| merge_prefixed_attribute_hashes(*hashes, prefix: :data, plural_keys: PLURAL_DATA_ATTRIBUTES) | ||
| end |
Contributor
There was a problem hiding this comment.
We could also think about moving that to the PVC repo directly...
Contributor
Author
I don't seem to be able to get a green build, but also can't reproduce these failures locally. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket
https://community.openproject.org/wp/OP-19415
What are you trying to accomplish?
Replaces
Filters::FilterForm(anApplicationFormsubclass) withFilters::FilterFormComponent(anApplicationComponent). The old form overrode:nodoc:Primer hooks (before_render,perform_render) and read semi-public ivars (@builder,@view_context). The new component receives the builder as an explicit keyword arg and uses a standard ERB template, reducing Primer internal coupling from five semi-public APIs to one (FormList).Screenshots
No visible changes.
What approach did you choose and why?
The component builds its controller wrapper with
merge_data, but upstreamPrimer::AttributesHelperdoes not treatcontrolleras a plural data attribute, so a caller-supplieddata-controllerwas silently overwritten by the requiredfilter--filters-formcontroller. AddedOpPrimer::AttributesHelper, which includes the upstream helper and overrides onlymerge_datato treatcontrolleras plural — multiple Stimulus controllers now concatenate (data-controller="a b") instead of clobbering. The component includes this helper in place of the Primer one.Merge checklist