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

Fix Voucher Code Field Focus Issue #12634

Conversation

chahmedejaz
Copy link
Collaborator

@chahmedejaz chahmedejaz commented Jul 1, 2024

What? Why?

  • Closes Voucher code field at checkout only allows one character #12632
  • Currently, we are using the generic toggle_control_controller to enable the Apply button if the voucher code is present.
  • This controller focuses the control after performing some toggle action on it i.e. after enabling etc.
  • In this case, the control is a submit button (Apply), and when user enters the voucher code, and after that the controller enables the Apply button and focuses it. Hence, the focus is lost from the Voucher Code input field.
  • I assume the focus functionality was intended for text-related fields, so this PR adds the check such that if the control is a button, then it won't be focused. Hence, the currently focused field will remain focused.

What should we test?

  • On the checkout page, when entering the voucher code, the field should remain focused and let you fill in the entire code without re-clicking it to focus after each character.

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

- when disabling or enabling the control, we should only focus it if it's not a button.
@RachL RachL added the user facing changes Thes pull requests affect the user experience label Jul 1, 2024
@chahmedejaz chahmedejaz marked this pull request as ready for review July 1, 2024 10:11
@chahmedejaz chahmedejaz requested review from dacook and mkllnk July 1, 2024 10:12
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.

Good one, could you update the related test as well ? thanks !

@chahmedejaz
Copy link
Collaborator Author

Good one, could you update the related test as well ? thanks !

Ah, sorry about that. I missed the Jest specs. Let me update that and get back to you. Thanks.

@chahmedejaz chahmedejaz force-pushed the bugfix/12632-voucher-field-focus-issue branch from d52a559 to ced3408 Compare July 2, 2024 10:51
@chahmedejaz chahmedejaz requested a review from rioug July 2, 2024 19:35
@chahmedejaz
Copy link
Collaborator Author

Hi @rioug - Specs are updated here: ced3408. Please review and let me know if anything is missing. Thanks.

Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Great work, and thanks for updating the existing spec to cover this also 👍

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.

Awesome, It looks good now thanks !

@chahmedejaz chahmedejaz added the bug-s2 The bug is affecting any of the non-critical features described in S1 and there is no workaround. label Jul 2, 2024
@dacook dacook removed the request for review from mkllnk July 3, 2024 00:07
@RachL RachL added pr-staged-fr staging.coopcircuits.fr pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-fr staging.coopcircuits.fr pr-staged-au staging.openfoodnetwork.org.au labels Jul 3, 2024
@RachL
Copy link
Contributor

RachL commented Jul 3, 2024

I didn't manage to successfully stage this one :( Don't know if it is linked to the errors we are seeing in master?

@sigmundpetersen
Copy link
Contributor

@RachL I think this is due to the same out-of-memory issue on fr-staging.
A puma restart should suffice.

@sigmundpetersen
Copy link
Contributor

This is how Filipe did it earlier #12485 (comment)

@RachL
Copy link
Contributor

RachL commented Jul 3, 2024

@sigmundpetersen ah good to know, thanks! but Filipe has ssh access, I don't :( FYI however I've tried staging AU and it failed as well

@sigmundpetersen
Copy link
Contributor

sigmundpetersen commented Jul 3, 2024

Yes, same error on au-staging.
So either out of memory there as well, or there is another issue (on both)

@drummer83 drummer83 self-assigned this Jul 8, 2024
@drummer83 drummer83 added no-staging-AU A tag which does not trigger deployments, indicating a server is being used and removed no-staging-AU A tag which does not trigger deployments, indicating a server is being used labels Jul 8, 2024
@drummer83 drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Jul 8, 2024
@drummer83
Copy link
Contributor

Hi @chahmedejaz,
Thank you for fixing this s2. 💪

I was able to reproduce the bug BEFORE this PR. After staging your solution, I couldn't see it anymore.

So I can confirms that the bug is fixed. That's great! Thank you!! 🙏

Kazam_screencast_00004.webm

Ready to go! 🚀 🥳

@drummer83 drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Jul 8, 2024
@drummer83 drummer83 merged commit d60d29b into openfoodfoundation:master Jul 8, 2024
59 of 62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-s2 The bug is affecting any of the non-critical features described in S1 and there is no workaround. user facing changes Thes pull requests affect the user experience
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Voucher code field at checkout only allows one character
6 participants