🎛️ Replace base `ExpandingFormset` functionality with Stimulus FormsetController `w-formset` inc. initial adoption #11633

lb- · 2024-02-11T07:48:58Z

First stage of #7915

Overview

Introduces a new Stimulus class FormsetController with the identifier w-formset with the longer term plan for this to be used for the InlinePanel JS functionality also.
Introduces a refactor of our common code for Django's formsets that streamlines how we set the delete input widget and prepares some of the Stimulus data attributes for use in the templates. This is a new base mixin wagtail.admin.forms.formsets.BaseFormSetMixin (happy for a different name to be suggested).
Replaces only the basic expanding formset usage in Group edit/create views, Workflow edit/create (Page selection only) with the Stimulus usage. No changes to InlinePanel at all.
Most importantly, this new approach is CSP compliant, has validation error messaging feedback for developers and does not rely on any complex id string wrangling, with fully tested support for complex nested usage.
- Stimulus does most of this for us through the Stimulus Scopes feature.
- I've left all nested IDs in place in the templates as there is no major need to remove these for now, but they're no longer used for any JS behaviour.

Additional notes

Introduces event dispatching for the expanding formsets to align with the InlinePanel https://docs.wagtail.org/en/stable/reference/pages/panels.html#inline-panel-events (have not documented, I thought release notes would be enough for this as it's not really used by customised code).
Removes the reliance on jQuery animations by introducing a function (which we can share later) that uses a promise callback for 'hiding' after an animation/transition is completed on an element without having to hard-code how long we expect the animation to take. For example, we can change the selected class to be ...-200 and the hidden attribute will be set after 200ms instead of the current 300. This reduces a common issue where we get the animation timing and the attribute updating out of sync causing chunky animations or confusing delays.
Commits are broken up to be a step by step creation and adoption of this new code, hopefully making reviewing easier.
instead of style display none, now using hidden for when a child form is deleted. This is more appropriate for the UI as the element is essentially meant to be removed.
There will be a subtle change in behaviour between the current, if you add 1001 children in any of the usage below. Currently, this would fail on server submit, however, with this change the add button will simply not add any more. I can change this to introduce a new option to ignore min/max but it seemed to make sense to keep this initial adoption simpler even with that edge case behaviour difference.
Naming of 'formset' vs 'form_set'
- I have opted for FormSet in the Python class names, this is what Django uses.
- formsets.py for the file in our admin files, again, following the Django convention.
- FormsetController/w-formset (not FormSetController/w-form-set) in the Javascript code for brevity and alignment with our existing set up event names on the InlinePanel code (e.g. w-formset:added).
Naming of the 'removed' event vs (delete/deleted action/code).
- Django uses the convention removed for the event https://docs.djangoproject.com/en/5.0/ref/contrib/admin/javascript/ we use that same event name in our documentation for InlinePanel https://docs.wagtail.org/en/stable/reference/pages/panels.html#javascript-dom-events
- Django uses the names delete/deleted in the code.
- I have aligned with that approach in the formset, while it's a bit odd to have removed/removing in the event names I hope this clarifies why.

Checklist

Do the tests still pass?
Does the code comply with the style guide?
For Python changes: Have you added tests to cover the new/fixed behaviour?
For front-end changes: Did you test on all of Wagtail’s supported environments? Chrome, Safari, Firefox all manually tested
For new features: Has the documentation been updated accordingly? (Upgrade considerations added)

How to test

Workflow edit, workflow create - validate that the pages sections all update correctly when adding/removing.
Workflow edit for a disabled workflow, check the console has no errors and the content is not visible for page selection as per current code.
User group create/edit - validate the page permissions and other non-page permissions changes.
For the above, check things still work as expected on Save with a POST form failure.

squash-labs · 2024-02-11T07:49:06Z

Manage this branch in Squash

Test this branch here: https://lb-featureformset-controller-i-hijkj.squash.io

client/src/controllers/FormsetController.ts

lb- · 2024-02-11T10:22:20Z

client/src/controllers/FormsetController.ts

+      target.contains(input),
+    );
+
+    if (!deleteInput) {


Note. I could not easily write a unit test for this error case due to Stimulus not catching these errors.

hotwired/stimulus#711

I have manually tested this scenario though and it does correctly throw in the console without breaking the whole Controller behaviour.

lb- · 2024-02-11T10:26:53Z

...admin/templates/wagtailadmin/permissions/includes/collection_member_permissions_formset.html

            {% include "wagtailadmin/permissions/includes/collection_member_permissions_form.html" with form=formset.empty_form only %}
        </tr>
    </template>

    <p class="add">
-        <button class="button bicolor button--icon" id="id_{{ formset.prefix }}-ADD" value="Add" type="button">{% icon name="plus" wrapped=1 %}{% block add_button_label %}{% endblock %}</button>
+        <button class="button bicolor button--icon" id="id_{{ formset.prefix }}-ADD" type="button" data-action="w-formset#add">{% icon name="plus" wrapped=1 %}{% block add_button_label %}{% endblock %}</button>


Suggested change

<button class="button bicolor button--icon" id="id_{{ formset.prefix }}-ADD" type="button" data-action="w-formset#add">{% icon name="plus" wrapped=1 %}{% block add_button_label %}{% endblock %}</button>

<button class="button bicolor button--icon" id="id_{{ formset.prefix }}-ADD" value="Add" type="button" data-action="w-formset#add">{% icon name="plus" wrapped=1 %}{% block add_button_label %}{% endblock %}</button>

Accidentally removed value="Add". I actually think we should not be setting the value on these buttons as it's not used visually and I'm pretty sure it's not used by the server.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#value

Probably an investigation for a different issue.

lb- · 2024-02-29T11:30:25Z

Rebased after #11620 - we can now remove even more bespoke JS imports across the user group edit/create views. Yay.

lb- · 2024-03-17T21:04:18Z

While this PR is not a fix for #11760 it's a good note that developers are expecting all features to work across ModelViewSet / Snippet views without having to think about what JS to import.

Hence, I am keen to get on the task of migrating InlinePanel once this PR is in.

Replaces the core functionality in `client/src/components/ExpandingFormset/index.js` and the Delete callbacks in other usage of the `window.buildExpandingFormset` function. Provides limitations on add/delete once the min/max have been reached but does not set up any disabling of these buttons as this is currently in the remit of `InlinePanel`. Includes a full test suite for error handling, updating inputs and nested usage which leverages Stimulus' scope management. Does not rely on any id attribute structure or wrangling. Relates to wagtail#7915

- Leverage the new Django 4.0 feature `deletion_widget` for formsets to attach data attributes and make the input a HiddenInput for existing and 'blank' forms. - Add support for data attributes used by the new Stimulus FormsetController

- Use the Wagtail admin formset mixin to adopt new data attributes on core form parts - Update the edit/create templates to put the attributes on the remaining elements - Add basic unit tests for smoke testing data attributes - Remove CSP incompatible inline scripts approach - Remove the now unused standalone static build/imports for the group edit view as this will not be needed - Ensure that the edit workflow form does not render any JS or attributes when the pages are not actually editable (workflow disabled)

- Use the Wagtail admin formset mixin to adopt new data attributes on core form parts - Update the edit/create templates to put the attributes on the remaining elements - Add basic unit tests for smoke testing data attributes - Remove CSP incompatible inline scripts approach - Remove the now unused standalone static build/imports for the group edit view as this will not be needed - Update Eslint accordingly

- window.buildExpandingFormset and using the base class `ExpandingFormset` can now be deprecated. - Move the window global setting to core.js which avoids the need to add the import in editor_js, to align with other future deprecations in core.js - Add upgrade considerations in release notes - `ExpandingFormset` is still needed for other existing usage (InlinePanel/MultipleChooserPanel) - Add additional JSDoc and updates to Eslint to advise developers that this feature is not to be used for new code

lb- added status:Needs Review component:Panels javascript Pull requests that update Javascript code component:Stimulus labels Feb 11, 2024

lb- commented Feb 11, 2024

View reviewed changes

client/src/controllers/FormsetController.ts Show resolved Hide resolved

client/src/controllers/FormsetController.ts Show resolved Hide resolved

lb- force-pushed the feature/formset-controller-initial-adoption branch from fc9c794 to 4fa1a29 Compare February 11, 2024 08:58

lb- mentioned this pull request Feb 11, 2024

🎛️ Migrate InlinePanel & Expanding formset to Stimulus controller w-formset #7915

Open

lb- changed the title ~~🎛️ Migrate base ExpandingFormset functionality to Stimulus FormsetController w-formset inc. initial adoption~~ 🎛️ Replace base ExpandingFormset functionality with Stimulus FormsetController w-formset inc. initial adoption Feb 11, 2024

lb- commented Feb 11, 2024

View reviewed changes

lb- mentioned this pull request Feb 12, 2024

Nested orderable inline panels have the same sort arrows as their orderable parent #5919

Closed

lb- force-pushed the feature/formset-controller-initial-adoption branch from 4fa1a29 to dbf012b Compare February 13, 2024 08:04

lb- mentioned this pull request Feb 24, 2024

Remove the usage of global chooserUrls #11620

Merged

5 tasks

lb- force-pushed the feature/formset-controller-initial-adoption branch from dbf012b to 371f6e9 Compare February 29, 2024 11:29

gasman self-requested a review March 6, 2024 09:53

lb- force-pushed the feature/formset-controller-initial-adoption branch from 371f6e9 to bbad762 Compare March 21, 2024 20:38

lb- added 6 commits March 23, 2024 14:56

Prepare Wagtail admin formset mixin

89f01d0

- Leverage the new Django 4.0 feature `deletion_widget` for formsets to attach data attributes and make the input a HiddenInput for existing and 'blank' forms. - Add support for data attributes used by the new Stimulus FormsetController

TEMP: Clear cache

147aa91

lb- force-pushed the feature/formset-controller-initial-adoption branch from bbad762 to 147aa91 Compare March 23, 2024 04:58

lb- mentioned this pull request Mar 24, 2024

InlinePanel doesn't work with ModelViewSet #11760

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎛️ Replace base `ExpandingFormset` functionality with Stimulus FormsetController `w-formset` inc. initial adoption #11633

🎛️ Replace base `ExpandingFormset` functionality with Stimulus FormsetController `w-formset` inc. initial adoption #11633

lb- commented Feb 11, 2024 •

edited

squash-labs bot commented Feb 11, 2024

lb- Feb 11, 2024

lb- Feb 11, 2024

lb- commented Feb 29, 2024

lb- commented Mar 17, 2024

	<button class="button bicolor button--icon" id="id_{{ formset.prefix }}-ADD" type="button" data-action="w-formset#add">{% icon name="plus" wrapped=1 %}{% block add_button_label %}{% endblock %}</button>
	<button class="button bicolor button--icon" id="id_{{ formset.prefix }}-ADD" value="Add" type="button" data-action="w-formset#add">{% icon name="plus" wrapped=1 %}{% block add_button_label %}{% endblock %}</button>

🎛️ Replace base ExpandingFormset functionality with Stimulus FormsetController w-formset inc. initial adoption #11633

Are you sure you want to change the base?

🎛️ Replace base ExpandingFormset functionality with Stimulus FormsetController w-formset inc. initial adoption #11633

Conversation

lb- commented Feb 11, 2024 • edited

Overview

Additional notes

Checklist

How to test

squash-labs bot commented Feb 11, 2024

Manage this branch in Squash

lb- Feb 11, 2024

Choose a reason for hiding this comment

lb- Feb 11, 2024

Choose a reason for hiding this comment

lb- commented Feb 29, 2024

lb- commented Mar 17, 2024

🎛️ Replace base `ExpandingFormset` functionality with Stimulus FormsetController `w-formset` inc. initial adoption #11633

🎛️ Replace base `ExpandingFormset` functionality with Stimulus FormsetController `w-formset` inc. initial adoption #11633

lb- commented Feb 11, 2024 •

edited