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

Feature/bass 150/modernize ad layers part 2 #81

Conversation

jomurgel
Copy link

@jomurgel jomurgel commented Nov 9, 2021

Continuation of #80.

  • Ports and cleans up ad layers classes from v1 plugin.
  • Replaces enqueue script with hashing solution.
  • Replaces Gulp with Webpack.
  • General cleanup and improvements.
  • Adds phpcs task for GitHub Actions.
  • Adds PHPUnit tests.

Copy link
Contributor

@renatonascalves renatonascalves left a comment

Choose a reason for hiding this comment

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

🗡️

A few suggestions but nothing blocking it. :)

.github/workflows/phpcs.yml Show resolved Hide resolved
.github/workflows/phpcs.yml Outdated Show resolved Hide resolved
.github/workflows/phpcs.yml Outdated Show resolved Hide resolved
.github/workflows/phpcs.yml Outdated Show resolved Hide resolved
*
* Should be implemented by all child classes.
*
* @access public
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually find those @access tags unnecessary since one can see quickly they are public/private/protected. They are easy to search for in an IDE. I'm not sure how helpful they actually are.

Copy link
Author

Choose a reason for hiding this comment

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

The majority of these I pulled directly from the old plugin. I'm going to hold onto this suggestion and the two below and tackle those in a 3rd PR.

But agreed, they're a little all over the place as to whether or not they're declared. So I think probably I'll remove them.

inc/ad-servers/class-ad-layers-dfp.php Show resolved Hide resolved
*
* @return array
*/
protected function get_ad_details() {
Copy link
Contributor

@renatonascalves renatonascalves Nov 10, 2021

Choose a reason for hiding this comment

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

Lots of methods could be type-casted since you are using PHP 7.4.

https://github.com/alleyinteractive/ad-layers/blob/75bd3d57a42f131ae882de97863e94fc3717141e/.phpcs.xml could also be updated to reflect that PHP version.

Suggested change
protected function get_ad_details() {
protected function get_ad_details(): array {

Copy link
Author

Choose a reason for hiding this comment

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

This is a good point. We were supporting 5.8 because this is an open-source plugin, I assume. I might figure out what options we have there.

@jomurgel jomurgel merged commit ec8c39f into feature/BASS-150/modernize-ad-layers-part-1 Nov 10, 2021
@jomurgel jomurgel deleted the feature/BASS-150/modernize-ad-layers-part-2 branch November 10, 2021 14:29
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.

2 participants