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

🎛️ RFC 78 - Initial Stimulus adoption v2 #9337

Merged

Conversation

lb-
Copy link
Member

@lb- lb- commented Oct 13, 2022

RFC 78 - Initial Stimulus adoption

This PR is a draft for the initial first set of commits to get Stimulus started in Wagtail.

Overview

  • This contains one Stimulus controller for the auto-submit form behaviour in images/documents listing drop-downs (e.g. changing the collection will submit the form) and is intentionally a simple case to get initial feedback
  • This is a partial re-implementation of https://github.com/lb-/wagtail/pull/5 - a more fleshed out adoption with more code usage (header search, skip link, upgrade notification).
  • This is also a smaller scoped version of 🎛️ RFC 78 - Initial Stimulus adoption #9075 (no docs, and no 'magic' controller inclusions).
  • This intentionally includes no documentation for now except for a folder README.md (planned new convention discussed within the UI team) to help guide contributors.
  • There are three non-core commits, two of them ignore some linting so that we can adopt Stimulus without more global ignoring of rules we should ignore, it also contains a fix for the draftail types that was causing CI failures previously.
  • Builds on Eslint - disable rule class-methods-use-this #9482 & Eslint - disable rule max-classes-per-file #9483

How to evaluate this PR

  1. If you have not already, read the RFC RFC 78: Adopt Stimulus (lightweight JS framework) 🎛️ rfcs#78
  2. Read the Stimulus docs (or at least the first few pages of the 'handbook' section) https://stimulus.hotwired.dev/handbook/origin
  3. Look at the granular commits, each builds on each other to adopt Stimulus in the code
  4. Run the code locally npm install npm start will be required and check the behaviour of the drop-downs in the images/documents listing actually works
  5. Read through the documentation and see if you can add your own controller easily within the bakerydemo
  6. If the feedback is general - best to put it on the RFC - otherwise feel free to put implementation specific feedback on this PR and it will be incorporated back into the RFC in due course.

@squash-labs
Copy link

squash-labs bot commented Oct 13, 2022

Manage this branch in Squash

Test this branch here: https://lb-featurerfc78-001-initial-st-bgqfq.squash.io

@lb-
Copy link
Member Author

lb- commented Oct 25, 2022

With RFC 78 merged - will mark this as ready for review.

@zerolab zerolab added status:Needs Review javascript Pull requests that update Javascript code labels Oct 25, 2022
@lb-
Copy link
Member Author

lb- commented Oct 31, 2022

Stimulus 3.1.1 out. Should update - https://github.com/hotwired/stimulus/releases/tag/v3.1.1

@lb- lb- force-pushed the feature/RFC78-001-initial-stimulus-adoption-v2 branch from 59a2276 to 4b32e2b Compare November 6, 2022 10:50
@lb-
Copy link
Member Author

lb- commented Nov 6, 2022

Rebased & updated to Stimulus 3.1.1

@thibaudcolas thibaudcolas self-requested a review November 16, 2022 15:16
@thibaudcolas thibaudcolas added this to the 4.2 milestone Nov 17, 2022
@thibaudcolas thibaudcolas self-assigned this Nov 17, 2022
@lb-
Copy link
Member Author

lb- commented Nov 28, 2022

Stimulus 3.2.0 is now out - making it easier to write keyboard shortcuts (without an external library), a nice outlets API (will help for things like bulk actions and search results) and a way to call a static method on registration of the controller (will help for backwards compatible global function creation).

https://github.com/hotwired/stimulus/releases/tag/v3.2.0

I'll aim to update and rebase this PR soon.

@lb- lb- force-pushed the feature/RFC78-001-initial-stimulus-adoption-v2 branch from 4b32e2b to bcfa2fc Compare November 28, 2022 22:01
@lb-
Copy link
Member Author

lb- commented Nov 28, 2022

Rebased & updated to Stimulus 3.2

@lb-
Copy link
Member Author

lb- commented Nov 28, 2022

CI failure is unrelated to this change.

Starting postgres service container
  /usr/bin/docker pull postgres:11
  Error response from daemon: Head "https://registry-1.docker.io/v2/library/postgres/manifests/11": received unexpected HTTP status: 503 Service Unavailable
  Warning: Docker pull failed with exit code 1, back off 2.89 seconds before retry.
  /usr/bin/docker pull postgres:11
  Error response from daemon: Head "https://registry-1.docker.io/v2/library/postgres/manifests/11": received unexpected HTTP status: 503 Service Unavailable
  Warning: Docker pull failed with exit code 1, back off 2.837 seconds before retry.
  /usr/bin/docker pull postgres:11
  Error response from daemon: Head "https://registry-1.docker.io/v2/library/postgres/manifests/11": received unexpected HTTP status: 503 Service Unavailable
  Error: Docker pull failed with exit code 1

@lb- lb- force-pushed the feature/RFC78-001-initial-stimulus-adoption-v2 branch from bcfa2fc to 69d555c Compare November 29, 2022 22:16
@lb-
Copy link
Member Author

lb- commented Nov 30, 2022

Another release - will try to update if I get the time. https://github.com/hotwired/stimulus/releases/tag/v3.2.1

Just a bug fix

@lb- lb- force-pushed the feature/RFC78-001-initial-stimulus-adoption-v2 branch from 69d555c to 79374eb Compare November 30, 2022 08:52
@pySilver
Copy link
Contributor

pySilver commented Dec 7, 2022

Just a side note. I'm working with Django & Stimulus (well it's not binded in any way) and I'm so so so so happy with it. HTML-over-the-wire it is so simple and flexible approach. I'm very happy it's going to be used inside Wagtail (one of the best CMS/F on the market). It would be much easier to extend Admin panel having Stimulus integrated.

@lb-
Copy link
Member Author

lb- commented Dec 9, 2022

Thanks @pySilver - very glad to hear that - if you have not already, be sure to have a read of the early draft documentation in the RFC.

Here is the direct link https://github.com/wagtail/rfcs/blob/main/text/078-adopt-stimulus-js.md#a-documentation-for-developers

The target audience of that section is those using Wagtail and wanting to customise parts of the admin and/or use the Stimulus application instance provided for their own JS widgets. With or without a Node tooling setup.

@lb- lb- force-pushed the feature/RFC78-001-initial-stimulus-adoption-v2 branch from 79374eb to 16afa19 Compare December 9, 2022 07:47
@lb-
Copy link
Member Author

lb- commented Dec 9, 2022

Rebased on latest (a few items have been merged in) thanks Thibuad.

Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

All working exactly as intended as far as I can tell! There are a few things I’d like to suggest simplifications for, keen to hear what you think.

.eslintrc.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
client/webpack.config.js Outdated Show resolved Hide resolved
client/src/controllers/AutoFieldController.ts Outdated Show resolved Hide resolved
client/src/controllers/AutoFieldController.ts Outdated Show resolved Hide resolved
client/src/includes/stimulus.ts Outdated Show resolved Hide resolved
client/src/includes/stimulus.test.js Outdated Show resolved Hide resolved
client/src/includes/stimulus.test.js Outdated Show resolved Hide resolved
client/src/includes/stimulus.test.js Outdated Show resolved Hide resolved
client/src/includes/stimulus.test.js Outdated Show resolved Hide resolved
@lb- lb- force-pushed the feature/RFC78-001-initial-stimulus-adoption-v2 branch 3 times, most recently from 2b0ab9e to 48d5195 Compare December 14, 2022 22:18
Copy link
Member Author

@lb- lb- left a comment

Choose a reason for hiding this comment

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

@thibaudcolas - this is ready for another review.

I've added one comment for myself, the JSDoc is alll cleaned up also and I've intentionally avoided an example of createController / Wagtail hook usage for now

client/src/controllers/README.md Show resolved Hide resolved
@lb- lb- force-pushed the feature/RFC78-001-initial-stimulus-adoption-v2 branch from 48d5195 to 5c72740 Compare December 15, 2022 21:30
- used to provide the ability for an input element to submit its form once changed or interacted with
@lb- lb- force-pushed the feature/RFC78-001-initial-stimulus-adoption-v2 branch from 5c72740 to f4d91c4 Compare December 16, 2022 07:16
Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

I’m surprised you removed all of the initialisation events – aside from that all good!

Note while testing this I got a PropTypes error from Storybook saying the React checked prop was submitted as the string "true" rather than a boolean. As far as I can tell this is a Storybook bug but thought I’d mention just in case.

Tested in Chrome 108 macOS 13, Firefox 102 Windows 11, Edge 102 Windows 11, Safari 14.1 macOS 12.1.

source-map-explorer output with Stimulus added for reference (nothing outstanding): https://leafy-centaur-e7a959.netlify.app/

@thibaudcolas thibaudcolas merged commit 84e2390 into wagtail:main Dec 19, 2022
@laymonage
Copy link
Member

laymonage commented Dec 19, 2022

Aaand it's in! Thank you very much @lb- and @thibaudcolas, can't wait to use this for new code and some refactoring 🥳

@lb- lb- deleted the feature/RFC78-001-initial-stimulus-adoption-v2 branch December 20, 2022 08:47
@lb-
Copy link
Member Author

lb- commented Dec 20, 2022

@thibaudcolas thank you for the thoughtful feedback on this, it's great to see it go in.

As for the events - I figured they may become helpful in the future, if so we can add them if needed. I've got a backup of the branch before removing them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants