-
Notifications
You must be signed in to change notification settings - Fork 3
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
Promos Adjustments #251
Promos Adjustments #251
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've done a first pass just looking at the code. There are a few logic things, but mainly I want to promote the use of the ternary operator as much as possible. We can discuss how that might look like with large HTML blocks to make it readable.
There are a lot of commits in this PR. If you want to start looking at merging the chore
commits to their respective commits. That would be good. Otherwise, we can leave that for the end.
@carlalexander I've addressed the issues you pointed out. Let me know if you think there are more things that could be improved. Thank you! |
@abaicus This all looks great. Thank you! Next, we should try to merge some of the commits together and clean up the commit history as I mentioned:
|
db574a0
to
0771374
Compare
@carlalexander It should be fine now. Some commits were mislabeled as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a lot better! I'd rather we didn't have a commit for the code review changes, but this PR is too big.
Another thing I noticed is that all the fix
commits didn't have any tests. Not sure if there's a large test suite in the SDK. But something I'll look at more next time.
Great work!! 🙌
🎉 This PR is included in version 3.3.28 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Changes summary:
Testing instructions:
Tip
You can use the debug filter to show all the notices available in the SDK. Just add the filter below to a mu-plugin, or use the QA plugin built in the comment to this PR.
add_filter( 'themeisle_sdk_promo_debug', '__return_true' );
CF7 Redirection notice:
Neve Notice:
themes.php
admin page;PPOM & Sparks notices:
Optimole notices:
Closes Codeinwp/themeisle#1654
Closes Codeinwp/themeisle#1655
Closes Codeinwp/themeisle#1656