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

Move rendering of Order Attribution inputs fully to JS #44335

Merged
merged 10 commits into from Feb 27, 2024

Conversation

tomalec
Copy link
Member

@tomalec tomalec commented Feb 3, 2024

This is a bit more revolutionary version of #44333, that aims to make the OA more resilient to bugs like #44159

Submission Review Guidelines:

Changes proposed in this Pull Request:

Addresses #44159

Before, the responsibility to manipulate DOM in regards to Order Attribution data for the classic forms was spread between PHP and JS. This was IMHO making the code harder to maintain, as a developer needs to jump between files to understand the logic. It also occurred to be pretty bug-prone, as we have at least two reports on some mismatch, where the JS is in place expecting the inputs to be rendered by PHP, but for some reason, they are not fully rendered.

Such change also adds the support for more complex use cases when there are multiple checkout and/or registration forms on the same page.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Fix the bugs
  1. Reproduce any of the issues mentioned above (TBD)
  2. Check that there are no errors
Multiple forms
  1. Add two classic checkout forms on the same page
  2. Proceed with shopping to this page
  3. In browser dev tools call: document.querySelectorAll( `wc-order-attribution-inputs` )
    • It should report return two elements
    • Both filled with inputs and data:
      image

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Move the rendering of Order Attribution inputs fully to JS. Support multiple instances on the same page.

Comment

in case the checkout form does not contain `woocommerce_checkout_shipping`, `woocommerce_after_order_notes`.

Addresses part of #44159
to move DOM manipulations  to JS only.
To support multiple checkout & register forms on the same page.

Addresses #44159
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Feb 3, 2024
Copy link
Contributor

github-actions bot commented Feb 3, 2024

Test Results Summary

Commit SHA: 9b25355

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 38s
E2E Tests326001703437m 28s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@tomalec
Copy link
Member Author

tomalec commented Feb 5, 2024

In the issue thread, @RistoNiinemets mentioned:

Maybe <wc-order-attribution-inputs>? 😄

Do you mean to rename the element's name to plural with "s" at the end?

I was thinking about it but couldn't decide ;)
Ideally, I'd make it, like:

window.customElements.define( 'wc-order-attribution-input', class extends HTMLElement {
	set values( values ) {
			this.#internals.setFormValue(  );
		}
	}

So, there would be just a single <wc-order-attribution-input></wc-order-attribution-input> that submits all the field values at once without sprouting multiple inputs.

But, as setFormValue is not going to land in Safari any soon, maybe it's better to call it what it is, not what we wish. ;)

@tomalec tomalec marked this pull request as ready for review February 5, 2024 18:40
@tomalec tomalec requested a review from a team February 5, 2024 18:40
Copy link
Contributor

github-actions bot commented Feb 5, 2024

Hi , @woocommerce/ventures

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@tomalec tomalec force-pushed the fix/44159-oa-missing-fields-element branch from bd0fca0 to eceab9c Compare February 5, 2024 18:43
@tomalec tomalec self-assigned this Feb 8, 2024
Base automatically changed from fix/44159-oa-missing-fields to trunk February 21, 2024 15:38
Copy link
Contributor

@layoutd layoutd left a comment

Choose a reason for hiding this comment

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

👍🏻 Thanks @tomalec, this works as described.

I noticed one small typo.

@tomalec tomalec closed this Feb 26, 2024
@tomalec tomalec reopened this Feb 26, 2024
@tomalec tomalec changed the title Improve rendering of Order Attribution inputs, move it fully to JS Move rendering of Order Attribution inputs fully to JS Feb 27, 2024
@tomalec tomalec merged commit c0750d5 into trunk Feb 27, 2024
41 checks passed
@tomalec tomalec deleted the fix/44159-oa-missing-fields-element branch February 27, 2024 11:31
@github-actions github-actions bot added this to the 8.8.0 milestone Feb 27, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Feb 27, 2024
@alvarothomas alvarothomas removed the needs: analysis Indicates if the PR requires a PR testing scrub session. label Feb 28, 2024
@alvarothomas alvarothomas added needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris labels Feb 28, 2024
Konamiman pushed a commit that referenced this pull request Mar 13, 2024
- Define custom element for order-attribution
   to move DOM manipulations to JS only.
   To support multiple checkout & register forms on the same page.

Co-authored-by: Justin Palmer <228780+layoutd@users.noreply.github.com>
Co-authored-by: github-actions <github-actions@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: internal testing Indicates if the PR requires further testing conducted by Solaris plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants