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

Prevent refresh on enter in header search (documents and images) #8119

Merged
merged 1 commit into from Mar 18, 2022

Conversation

rd3m
Copy link

@rd3m rd3m commented Mar 15, 2022

References issue #3746

@rd3m rd3m marked this pull request as ready for review March 15, 2022 08:52
@squash-labs
Copy link

squash-labs bot commented Mar 15, 2022

Manage this branch in Squash

Test this branch here: https://prevent-refresh-on-enter-1zus4.squash.io

@lb-
Copy link
Member

lb- commented Mar 15, 2022

@rd3m
Not sure that a JavaScript solution is ideal here, maybe we should explore something like this
https://stackoverflow.com/a/51507806/8070948

@thibaudcolas thibaudcolas added the GSOC Google Summer of Code label Mar 16, 2022
@rd3m
Copy link
Author

rd3m commented Mar 17, 2022

Hi @lb-, yes, that's a much nicer solution.

@lb-
Copy link
Member

lb- commented Mar 18, 2022

not for review - need to re-check keyboard and screen reader accessibility for this change

@lb- lb- force-pushed the prevent-refresh-on-enter branch from 8d6e8ef to ef00de6 Compare March 18, 2022 12:31
- resolves wagtail#3746
- add a hidden & disabled first submit input so that ‘enter’ does not trigger submit
- allow the behaviour of the actual submit button to still work as expected
- intentionally HTML only solution as it is simpler and more accessible than JS override
@lb- lb- force-pushed the prevent-refresh-on-enter branch from ef00de6 to 13b1209 Compare March 18, 2022 12:32
Copy link
Member

@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.

Looks great @rd3m - thanks for taking the time to try out some different solutions.

Keyboard control still works when focused on the hidden button (the non-disabled one) and screen reader interaction still works.

Thanks for making your first contribution to Wagtail!

I have put your name on the changelog and contributors list as (Riley de Mestre) - please let me know if you would like your name listed differently.

@lb-
Copy link
Member

lb- commented Mar 18, 2022

Added changelog/pushed to remote branch.

@lb- lb- merged commit 074ea8c into wagtail:main Mar 18, 2022
lb- added a commit that referenced this pull request Mar 18, 2022
@rd3m
Copy link
Author

rd3m commented Mar 18, 2022

First contribution, very exciting, thanks @lb- !

thibaudcolas added a commit to stevedya/wagtail that referenced this pull request Mar 23, 2022
* main: (31 commits)
  Removed outdated handling of length parameter to If-Modified-Since header
  styles - disable text-transform property-disallowed-list for required usage
  update @wagtail/stylelint-config-wagtail npm package
  disabled text-transform tailwind plugin
  removed redundant text-transform instances
  format readme with prettier
  Add some more dummy modules
  Add rule into upgrademodulepaths command for rewriting wagtail.tests imports
  Add some dummy modules for wagtail.tests
  Add a replacement rule into `wagtail updatemodulepaths` for the wagtail.core rename
  fix typo from wagtail#8119
  prevent page refresh on enter key in header seach
  Add some more dummy modules for moved wagtail.core modules
  Update node installation instructions (wagtail#8154)
  Don't crash if wagtail/__init__.py is loaded without Django installed
  Add dummy modules to maintain wagtail.core imports
  Rename WagtailCoreAppConfig to WagtailAppConfig
  Move wagtail.core to wagtail
  Move tests to test
  Merge sites utilities into sites models
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants