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

Migrate header search to a Stimulus controller (basic version) #9952

Merged
merged 1 commit into from Jul 6, 2023

Conversation

lb-
Copy link
Member

@lb- lb- commented Jan 24, 2023

  • Removes the jQuery slide animation so content will be instantly replaced (we do not appear to have the animation for content replacement in the search results within modals and other similar places so I assumed it was not something worth rebuilding)
  • Removes the autofocus behaviour on the search fields as this is not helpful for screen readers / keyboard control
  • Sets the aria-busy attribute on the results container while loading
  • Includes support for window.header if provided
  • Base implementation for 🎛️ Migrate header search Stimulus Controller #9950

  • Do the tests still pass?
  • Does the code comply with the style guide?
  • For front-end changes: Did you test on all of Wagtail’s supported environments? Chrome 109, Firefox 109, Safari 16.1 on macOS 13.0

@lb- lb- added component:Search javascript Pull requests that update Javascript code labels Jan 24, 2023
@squash-labs
Copy link

squash-labs bot commented Jan 24, 2023

Manage this branch in Squash

Test this branch here: https://lb-featurestimulus-header-sear-zipx1.squash.io

@lb- lb- force-pushed the feature/stimulus-header-search branch 4 times, most recently from 87f095b to 338b70e Compare January 24, 2023 21:52
lb- added a commit to lb-/wagtail that referenced this pull request Jan 24, 2023
lb- added a commit to lb-/wagtail that referenced this pull request Jan 24, 2023
@lb-
Copy link
Member Author

lb- commented Jan 26, 2023

Thinking that w-inject may be better for this controller - as it's not just for search but rather injection of html from a response.

Other name ideas; w-frame (inspired from Turbo) or w-swap (inspired from HTMX) or w-update.

@lb- lb- force-pushed the feature/stimulus-header-search branch from 338b70e to 4b63541 Compare February 1, 2023 22:13
@lb-
Copy link
Member Author

lb- commented Feb 1, 2023

@thibaudcolas I have cleaned this up a bunch more - would love to get your initial pass on feedback for this when you get the chance.

@lb- lb- force-pushed the feature/stimulus-header-search branch 3 times, most recently from 6327403 to b54a838 Compare February 4, 2023 03:12
@lb-
Copy link
Member Author

lb- commented Feb 4, 2023

@thibaudcolas I have updated this implementation to set aria-busy on the target container while 'loading', I opted out of setting aria-live="polite" on the elements though as it appears we have inconsistent header implementations so we end up with confusing screen reader output.

Happy to fix this also but I think it may increase the scope of this PR, happy to raise a new issue once this is merged.

Example (with aria-live)

match results - works great as that uses a h2 on the results with role=alert.

Screenshot 2023-02-04 at 1 09 20 pm

It also reads 'busy' if the screen reader has 'time'.

no results - works great as that uses a h2 on the results also

Screenshot 2023-02-04 at 1 16 47 pm

clear search - does not work well at all, it reads the entire content (no h2)

https://github.com/wagtail/wagtail/blob/main/wagtail/images/templates/wagtailimages/images/results.html#L5

Screenshot 2023-02-04 at 1 17 39 pm

@lb- lb- requested a review from thibaudcolas February 4, 2023 03:26
@lb- lb- force-pushed the feature/stimulus-header-search branch from b54a838 to d521591 Compare February 4, 2023 03:42
lb- added a commit to lb-/wagtail that referenced this pull request Feb 4, 2023
@lb-
Copy link
Member Author

lb- commented Jul 4, 2023

@laymonage ok should be good for another review.

I can do another smoke test for the aria-busy, I do recall (been a while) I had to slow down the voice read rate but I did validate that the screen reader correctly read out that the content had been updated.

As for the legacy window.headerSearch support, I have added more support here with an afterLoad method, so that if any window.headerSearch is used AND its termInput finds an input without a controller attached to its form. Then it will set up the right data attributes automatically.

I still think it's not worth doing the upgrade notes as part of this PR though, I hope to get another PR done after this one which will remove ALL window.headerSearch inline script usage and replace with the data attribute approaches. However, the goal is to have this PR be able to go to release if that next PR is not yet done.

@lb- lb- requested a review from laymonage July 4, 2023 21:35
@lb- lb- force-pushed the feature/stimulus-header-search branch 2 times, most recently from 65c55d3 to 5756c17 Compare July 5, 2023 00:54
lb- added a commit to lb-/wagtail that referenced this pull request Jul 5, 2023
Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

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

A few more things that I should've noticed earlier (sorry!), and some comments on the naming before we get this in (as it'll be harder to change later). Once those are resolved, I think this should be ready to be merged 😄

client/src/controllers/SwapController.ts Outdated Show resolved Hide resolved
client/src/controllers/SwapController.ts Outdated Show resolved Hide resolved
client/src/controllers/SwapController.ts Show resolved Hide resolved
client/src/controllers/SwapController.ts Outdated Show resolved Hide resolved
client/src/controllers/SwapController.ts Show resolved Hide resolved
client/src/controllers/SwapController.ts Outdated Show resolved Hide resolved
@lb- lb- force-pushed the feature/stimulus-header-search branch from d4c8773 to 41c6b33 Compare July 5, 2023 12:01
@lb-
Copy link
Member Author

lb- commented Jul 5, 2023

@laymonage ready for another round, thank you for taking the time to review this thoroughly. I feel like this is a really fine tuned approach now, let me know if anything stands out.

@lb- lb- requested a review from laymonage July 5, 2023 12:03
@lb- lb- force-pushed the feature/stimulus-header-search branch 2 times, most recently from b5a7108 to 5dc2363 Compare July 5, 2023 12:14
lb- added a commit to lb-/wagtail that referenced this pull request Jul 5, 2023
Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

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

Looking very solid here, thanks for continuing to work on this! So happy to see all the controller code covered by tests. One small typo but I think that can be fixed as you merge this. (I can also merge if you want.)

Thanks again!

client/src/controllers/SwapController.ts Outdated Show resolved Hide resolved
@lb- lb- force-pushed the feature/stimulus-header-search branch from 8f67a3a to 025425e Compare July 6, 2023 10:08
- Removes the jQuery slide animation so content will be instantly replaced
- Removes the autofocus behaviour on the search fields as this is not helpful for screen readers / keyboard control
- Includes support for `window.header` if provided alongside dynamic adding of data-* attributes if not included
- Base implementation for wagtail#9950
- Co-authored-by: sag᠎e <laymonage@gmail.com>
@lb- lb- force-pushed the feature/stimulus-header-search branch from 025425e to 0d09e4a Compare July 6, 2023 10:09
@lb- lb- merged commit 04d1e81 into wagtail:main Jul 6, 2023
11 of 15 checks passed
@lb- lb- deleted the feature/stimulus-header-search branch July 6, 2023 10:10
@lb-
Copy link
Member Author

lb- commented Jul 6, 2023

Thank you @laymonage - I know it's a big job to review some of these PRs.

I am excited to see us start to have a baseline of shared, reusable JS behaviours built with a similar approach... and unit tested :D

lb- added a commit to lb-/wagtail that referenced this pull request Jul 6, 2023
@laymonage laymonage added this to the 5.1 milestone Jul 11, 2023
lb- added a commit to lb-/wagtail that referenced this pull request Jul 12, 2023
- Builds on wagtail#9952
- Create a new method `submit` and `submitLazy` to serialise a form's inputs and submit (GET) async to replace content
- Create a lazy version of `replace` and add unit tests for it
- Partial progress on wagtail#9950
lb- added a commit to lb-/wagtail that referenced this pull request Jul 13, 2023
- Builds on wagtail#9952
- Create a new method `submit` and `submitLazy` to serialise a form's inputs and submit (GET) async to replace content
- Create a lazy version of `replace` and add unit tests for it
- Partial progress on wagtail#9950
lb- added a commit to lb-/wagtail that referenced this pull request Jul 15, 2023
- Builds on wagtail#9952
- Create a new method `submit` and `submitLazy` to serialise a form's inputs and submit (GET) async to replace content
- Create a lazy version of `replace` and add unit tests for it
- Partial progress on wagtail#9950
lb- added a commit to lb-/wagtail that referenced this pull request Jul 17, 2023
- Builds on wagtail#9952
- Create a new method `submit` and `submitLazy` to serialise a form's inputs and submit (GET) async to replace content
- Create a lazy version of `replace` and add unit tests for it
- Partial progress on wagtail#9950
lb- added a commit to lb-/wagtail that referenced this pull request Jul 17, 2023
- Builds on wagtail#9952
- Create a new method `submit` and `submitLazy` to serialise a form's inputs and submit (GET) async to replace content
- Create a lazy version of `replace` and add unit tests for it
- Partial progress on wagtail#9950
lb- added a commit that referenced this pull request Jul 17, 2023
- Builds on #9952
- Create a new method `submit` and `submitLazy` to serialise a form's inputs and submit (GET) async to replace content
- Create a lazy version of `replace` and add unit tests for it
- Partial progress on #9950
lb- added a commit to lb-/wagtail that referenced this pull request Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:Search javascript Pull requests that update Javascript code
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants