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

Replace toggle_button_disable controller with generic toggle_control controller #12050

Conversation

cyrillefr
Copy link
Contributor

  • add enableIfPresent method in toggle_control_controller.js` to handle enabling on toggle
  • add testing in the corresponding spec
  • replace in view previous controller with intended generic toggle-control

What? Why?

See #11928
Does not close the issue, as this is a replacement for the first of the three old/tailored/legacy controllers.

The replaced controller was used only to toggle disability of the button to add a voucher at the checkout.

I have forgot to remove the previous controller and its spec though ...

What should we test?

Make a checkout and see if button to add voucher is disabled at first and then enabled when data is entered.
Or this trick:
rspec spec/system/consumer/checkout/payment_spec.rb:128 with putting a binding.pry to stop the test and see the page where the checkout + voucher is made. Need to alter cuprite setting too (headless: false)

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

…ller

- add enableIfPresent method in toggle_control_controller.js to handle enabling on toggle
- add testing in the corresponding spec
- replace in view previous ctrler with intended generic toggle-control
@cyrillefr
Copy link
Contributor Author

Hello @dacook, could it be possible to rerun tests as they failed at "Install JS dependencies" ?

@sigmundpetersen
Copy link
Contributor

done @cyrillefr

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, this looks perfect to me!

Just a couple of things, one below, and as you mentioned:

remove the previous controller and its spec

Would you be able to add that as a new commit to this PR?

app/webpacker/controllers/toggle_control_controller.js Outdated Show resolved Hide resolved
- also delete one comment about replacing this controller with another
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.

Wonderful, thank you!

@cyrillefr
Copy link
Contributor Author

Hello @dacook ,

Could it be possible to merge this PR ?
I will have to make modifications to the same js controller file for the next PR that tackles #11928. (I am talking app/webpacker/controllers/toggle_control_controller.js that was modified).
I would rather avoid conflicts later :)

@dacook
Copy link
Member

dacook commented Jan 22, 2024

Hi @cyrillefr , sorry for the delay. As this affects the front end, we need to verify with a manual test before merging, just in case any unforeseen issues come up. We have a few items in the testing list at the moment so it may have to wait a little while.

In a case like this, and if you're confident with Git, I would suggest that you can create a new branch from this one, to start your next task. There's a small risk of conflicts if this branch will get changed after testing, but it seems unlikely in this case.

@cyrillefr
Copy link
Contributor Author

Hello @dacook ,

Thank you for the clarification. My goal was certainly not to rush anyone :)

Did not know I could on github make a branch from another one.
I will go on the simpler way and wait for this one to be merged to put the next step.

@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

Thank you, @cyrillefr!

I can confirm that the apply button for the voucher code is still working correctly.

Peek.2024-01-24.16-38.mp4

Great, I will merge this one and you can feel free to take the next step of this consolidation. 💪

Thanks again! Merging! 🥳

@drummer83 drummer83 merged commit 5377304 into openfoodfoundation:master Jan 24, 2024
52 checks passed
@drummer83 drummer83 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 24, 2024
@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
@cyrillefr cyrillefr deleted the Replace-toggle_button_disabled_controller-with-generic-toggle_control_controller branch February 26, 2024 10:09
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

5 participants