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 PHP support with foundation for PHP libraries to load third parties #40

Merged
merged 18 commits into from
Apr 3, 2024

Conversation

felixarntz
Copy link
Collaborator

@felixarntz felixarntz commented Dec 12, 2023

Fixes #39

  • PHP classes foundation
  • PHP third parties implementation
  • Test coverage
  • Add docs
  • Move JSON files out of JS-specific folders (will be done in a follow up PR)

@adamsilverstein
Copy link
Collaborator

@felixarntz - this looks great so far! the main possible improvement would be adding tests although I'm not sure that should be a blocker since the JS side doesn't have them :)

@flashdesignory
Copy link
Collaborator

@felixarntz - this looks great so far! the main possible improvement would be adding tests although I'm not sure that should be a blocker since the JS side doesn't have them :)

It's always great to have a new set of eyes on the JS side as well 😄
We do cover the crucial functions with unit tests (utils file), but please let us know if you feel that there are some tests missing.
Personally, I don't think we need 100% test coverage, but should cover the crucial parts to ensure we're delivering the correct output.

@felixarntz
Copy link
Collaborator Author

@housseindjirdeh @adamsilverstein @flashdesignory I've completed the remaining tests so this is now ready for full review.

Regarding documentation, for now I only added a small note to the readme referring to both the JS and PHP implementations. More wasn't really needed at this point, as everything else in the readme applies in both scenarios.

I think it would be useful to add more specific documentation (most importantly a few code examples in both JS and PHP), but that could be done in a separate PR.

@felixarntz felixarntz marked this pull request as ready for review January 13, 2024 01:53
@felixarntz
Copy link
Collaborator Author

@housseindjirdeh @adamsilverstein @flashdesignory Friendly ping :)

Any chance we can get this merged in the next 1-2 weeks?

@housseindjirdeh housseindjirdeh merged commit a295389 into main Apr 3, 2024
6 checks passed
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.

Add PHP support
4 participants