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.

Sign up for GitHub

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

Jump to bottom

Enhancement/4013 add notice for compat autoptimize aggregatecss rocket cpcss #4525

Merged
merged 29 commits into from
Nov 24, 2021
Merged

Enhancement/4013 add notice for compat autoptimize aggregatecss rocket cpcss #4525

merged 29 commits into from
Nov 24, 2021

Conversation

iCaspar
Copy link
Contributor

@iCaspar iCaspar commented Nov 23, 2021 •
edited
Loading

Description

Adds a warning notice to advise users of conflict between Autoptimize Aggregate Inline CSS and CPCSS ("Optimize CSS Delivery")

Fixes #4013

Type of change

  • Enhancement (non-breaking change which improves an existing functionality)
  • This change requires a documentation update

Is the solution different from the one proposed during the grooming?

Yes. This solution builds on the solution from #4501, using the Autoptimize compatibility class implemented there. It is no longer dependent on settings_transients.

How Has This Been Tested?

Integration tests have been added for the new notice method.
Verified functionality locally.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@iCaspar
Add tests and method for AO agg-js and wpr delayjs
4a2c99c
@iCaspar
Fix param order in docblock
b279d92
@iCaspar
Use add_settings_error instead of transient
a8b51e1
@iCaspar
Fix docblock order
138bbe4
@iCaspar
Remove dead transient_errrors code
75a7322
@iCaspar
Remove extra spacing
6b99789
@iCaspar
Use matching priority in test
44e9970
@iCaspar
Remove using void thing.
e1c6031
@iCaspar
Move warning method to new Compat Subscriber
2c83073
@iCaspar
Reconfigure for new story conditions
74659bf
@iCaspar
Add Autoptimize class to container
6b7a55c
@iCaspar
Remove dead code from DelayJs Subscriber
4240a87
@iCaspar
Move test to ThirdParty
b924a32
@iCaspar
Check failing test
0b65d66 
This will display the notice as expected, but the integration test will not run in isolation. Other functions from preload still run on admin_notice.
@iCaspar
Fix phpcs
e3a3db1
@iCaspar
Revise tests and make pass
45b6cec
@iCaspar
Fix open/close strong tags
b125877
@iCaspar
Fix strong tags in test
777de54
@iCaspar
Add tests and notice for AggInlineCSS and CPCSS
e38c769
@iCaspar
Add missing testcase for DelayJsAggregation
9c20790
@iCaspar
Fix extra (non-functional) dismiss icon
0c5222a
@iCaspar
Revise tests for no-icon warning
f134d63
@iCaspar
Remove extra dismiss icon
2c35105
@iCaspar
Merge branch 'enhancement/4472-add-notice-for-compat-autoptimize-aggr…
d7f304b 
…egatejs-rocket-delayjs' into enhancement/4013-add-notice-for-compat-autoptimize-aggregatecss-rocket-cpcss
@iCaspar
Remove unnecessary $errors property
3d26a61
@iCaspar iCaspar requested a review from a team November 23, 2021 15:23
@iCaspar iCaspar self-assigned this Nov 23, 2021
@iCaspar iCaspar added the 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting label Nov 23, 2021
@iCaspar
Merge branch 'develop' into enhancement/4013-add-notice-for-compat-au…
60bcbbc 
…toptimize-aggregatecss-rocket-cpcss

# Conflicts:
#	inc/ThirdParty/Plugins/Optimization/Autoptimize.php
#	tests/Fixtures/inc/ThirdParty/Plugins/Optimization/Autoptimize/warnWhenJsAggregationAndDelayJsActive.php
#	tests/Integration/inc/ThirdParty/Plugins/Optimization/Autoptimize/warnWhenJsAggregationAndDelayJsActive.php
@Mai-Saad Mai-Saad self-requested a review November 23, 2021 17:25
@Mai-Saad
Copy link
Contributor

@iCaspar @piotrbak
Now Notice is displayed even if Aggregate CSS-files is unchecked (at this point aggregate inline CSS is disabled with its previous state i.e if it was checked then it will be checked and dim) => in this case we normally apply CPCSS on existing files and I think no need for the notice in this case (probably same will be in similar issues). So what do you think?
Screenshot from 2021-11-23 19-59-11
Screenshot from 2021-11-23 19-58-52

@iCaspar
Copy link
Contributor Author

iCaspar commented Nov 23, 2021

On the one hand, the notice isn't really necessary in this case of dimmed sub-options. On the other, since we're not actually changing the behavior of the features, it won't actually break anything.
@piotrbak Shall we try to fix this now for 3.10.4, or later for 3.10.5?

@iCaspar
Copy link
Contributor Author

iCaspar commented Nov 23, 2021

@piotrbak @Mai-Saad
The last change should take into account the "up the chain" settings for both this and the previous (DelayJS/AggregateJS) notices. The last one (#4528) is the main CSS setting, so shouldn't be affected by this.

Mai-Saad
Mai-Saad approved these changes Nov 24, 2021
View reviewed changes
Copy link
Contributor

@Mai-Saad Mai-Saad left a comment

Choose a reason for hiding this comment

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

Working as expected

@engahmeds3ed engahmeds3ed merged commit 7b69a64 into develop Nov 24, 2021
@engahmeds3ed engahmeds3ed deleted the enhancement/4013-add-notice-for-compat-autoptimize-aggregatecss-rocket-cpcss branch November 24, 2021 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mai-Saad Mai-Saad Mai-Saad approved these changes

@remyperona remyperona remyperona approved these changes

Assignees

@iCaspar iCaspar

Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Autoptimize compatibility: make CPCSS compatible when using their "aggregate inline CSS" option
4 participants
@iCaspar @Mai-Saad @remyperona @engahmeds3ed