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

Hotkey fix #12052

Merged
merged 2 commits into from
Jan 24, 2024
Merged

Hotkey fix #12052

merged 2 commits into from
Jan 24, 2024

Conversation

dacook
Copy link
Member

@dacook dacook commented Jan 17, 2024

What? Why?

I found an issue with the ctrl+enter keyboard shortcut on a StimulusReflex form, and fixed it.

What should we test?

With admin_style_v3 enabled,

  1. visit /admin/products
  2. Edit a product
  3. press ctrl+enter (or cmd+enter on mac)

The change should be saved successfully.

Before, it would ask if you want to leave the site, and if you click yes, you got an error "Not found".

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

A StimulusReflex form handler was being ignored, resulting in an error.

Note that this method can support angular forms, the submit buttons just need to be updated to type='submit' (why they are not already boggles me).
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Interesting. I would argue that those forms should be fixed to use submit instead of click. There may be other problems with submitting. But this is a good workaround for now.

@dacook
Copy link
Member Author

dacook commented Jan 18, 2024

I would argue that those forms should be fixed to use submit instead of click.

Me too! It appears to be a very straightforward change, but there may be a reason it was set up that way. And there are a lot of forms to test, so I decided not to go down the rabbit hole this time ;)

Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Nice one !

@drummer83 drummer83 self-assigned this Jan 24, 2024
@drummer83 drummer83 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 24, 2024
@drummer83
Copy link
Contributor

Hi @dacook,
Thanks for this addition.

I was able to reproduce the "not found" message before staging your PR:
image

After staging we now have the same behavior on page /products for ENTER as well as CTRL + ENTER. Both are saving the changes - or throwing an error if there is one.

image

Great fix! Thanks again!
Merging! 🥳 🚀

@drummer83 drummer83 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 24, 2024
@drummer83 drummer83 merged commit d15395a into openfoodfoundation:master Jan 24, 2024
52 checks passed
@mkllnk mkllnk added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants