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

Add increased compatibility with custom checkout themes. #115

Closed
wants to merge 1 commit into from

Conversation

clifgriffin
Copy link
Contributor

  1. Gets rid of superfluous change trigger on fields during check_changes()
  2. Reduce selector rigidity in getSelectorContainerID() and getSelectedPaymentCategory()

1) Gets rid of superfluous change trigger on fields during check_changes()
2) Reduce selector rigidity in getSelectorContainerID() and getSelectedPaymentCategory()
@clifgriffin
Copy link
Contributor Author

Just checking in on this. 🙂

@NiklasHogefjord
Copy link
Contributor

Hi @clifgriffin,

Thanks for the PR and sorry for very late reply. We will look into it during this week.

@MichaelBengtsson can you check this a bit closer and see if we can merge it?

@MichaelBengtsson
Copy link
Contributor

@clifgriffin I will look at this for the next release of the plugin. On the 2nd point you made, that is perfectly fine, but the first one i need to make sure that the update you are calling is actually superfluous before its being removed. So i need to look at the code you sent in for that part before hand, which is why it was not done for the previous release

@clifgriffin
Copy link
Contributor Author

@MichaelBengtsson No problem. I should have documented more precisely the way that the change trigger was causing issues in our tests.

I'm generally wary of firing change events unless explicitly needed because they are usually a proxy for enforcing / maintaining state, but in a system where there can be lots of things watching for change events there can be unexpected side effects. (And there are other ways to do it, like explicitly calling your change handler)

Let me know if you need more information / justification from me. I don't 100% fully understand the purpose of check_changes() but in my debugging it didn't seem to be a necessary trigger.

@clifgriffin
Copy link
Contributor Author

Just checking on this! We have multiple customers on a patched version who would love to be able to make normal updates.

Thanks!

P.S. None of them have had any issues with changes to check_changes(). Just FYI.

@MichaelBengtsson
Copy link
Contributor

@clifgriffin Hi again, sorry for the delay.

I remember looking at the code and trying something similar or exactly what you are doing, and it caused further issues with some other themes. I can look into this some more this comming week.

About the check_changes() part, this has also been removed from the current version of the plugin. So yes, that was not a problem at all.

@clifgriffin
Copy link
Contributor Author

@MichaelBengtsson Thanks for the response! I'm really surprised by the theme compatibility issue. Are themes stripping the .wc_payment_method class?

If you wanted to send me a copy of one of the problem themes, I'd be happy to take a look at what we can do to make it work for everyone.

@MichaelBengtsson
Copy link
Contributor

@clifgriffin I might have been misremembering, and I might have tried a different class. If you could send the theme to us then that would be a good help. The main issue I can see making a change like this is that for some merchants where the plugin is currently working, it wont be working anymore if it causes an issue with their theme.

Ideally I would like to get this part of the plugin as theme independent as possible, and get away from HTML markup dependency altogether, since its causing a few issues with other plugins/themes in different ways.

@clifgriffin
Copy link
Contributor Author

@MichaelBengtsson Happy to send you a copy. Is there a way for me to send it to you privately? My email is clif [at] checkoutwc.com

Thanks!

@MichaelBengtsson
Copy link
Contributor

@clifgriffin I will send you an email and we can get in touch there.

@MichaelBengtsson
Copy link
Contributor

Closed. We made a change to the selector to something that should be more compatible with the 1.8.1 release. I have been in contact with @clifgriffin and this seemed to have solved the issue using the theme he was using before in my enviroment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants