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

Multiple Fixes #13

Merged
merged 16 commits into from
Jan 11, 2022
Merged

Multiple Fixes #13

merged 16 commits into from
Jan 11, 2022

Conversation

acornforth
Copy link
Contributor

  • Add option to use Province abbreviation instead of code for US state fields
  • Allow override of money conversion for sylius installations storing amounts with different precision.
  • fix translations typos (also covers Fix for Typo on Encryptation #11 )
  • tidy up some code smells
  • add ecs.php cofing file
  • fix some composer.json issues preventing install on newer php and sylius

@sbarbat
Copy link
Owner

sbarbat commented Aug 12, 2021

Hey @acornforth, thank you very much for your contribution 🎉 . Seems there are some merge conflicts with the last change we merged. Can you fix them to merge this?

Thanks

@acornforth
Copy link
Contributor Author

Happy to, give me a moment 👍

@acornforth
Copy link
Contributor Author

good to go

@euperia
Copy link
Contributor

euperia commented Jan 10, 2022

@sbarbat are there any plans to merge this PR? It will fix an issue I'm having with BillingState values and non-us addresses.

@sbarbat
Copy link
Owner

sbarbat commented Jan 10, 2022

It seems its formatting a lot of files that shouldn't, if you can solve it then would be merged 😄 @euperia

@acornforth
Copy link
Contributor Author

acornforth commented Jan 10, 2022

@sbarbat that is my bad for not limiting the scope of my PR to a single feature. I wanted to make that fix, but as i was working on the code i realized that there were a number of inconsitencies in the code style. To help keep code from all contributors consistent, i added a config file for Easy Coding Standard (already a dependency in sylius) with a basic rule-set, and applied all the suggested fixes.

Obviously as the maintainer it's up to you if you want to accept those changes, but i thought they were beneficial.

@euperia If you want to take advantage of these changes you could always use my forked repo ( i suggest you make your own fork) you can then configure composer to pull the package from your fork instead of this repo.

@sbarbat
Copy link
Owner

sbarbat commented Jan 11, 2022

@acornforth sorry it's been some time since I last developed with PHP so didn't remember about those standards.

If all the stylings are done using ECS, lets make sure we use that styling in future PR's.

Approving 😄

@acornforth thank you 🎉
@euperia thanks for the follow-up 🙏

@sbarbat sbarbat merged commit 9d116dd into sbarbat:master Jan 11, 2022
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.

3 participants