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

Closes #4320 combine CSS divi 4.10 compatibility #4377

Merged

Conversation

mostafa-hisham
Copy link
Contributor

@mostafa-hisham mostafa-hisham commented Sep 23, 2021

Description

exclude all Divi CSS files which are stored in /wp-content/et-cache/ From being Combined

Fixes #4320

Type of change

  • Enhancement (non-breaking change which improves an existing functionality)

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

No

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Tested 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

@mostafa-hisham mostafa-hisham requested a review from a team September 23, 2021 12:12
@mostafa-hisham mostafa-hisham self-assigned this Sep 23, 2021
@mostafa-hisham mostafa-hisham marked this pull request as ready for review September 23, 2021 14:16
@Mai-Saad Mai-Saad self-requested a review October 1, 2021 07:27
@Mai-Saad
Copy link

Mai-Saad commented Oct 1, 2021

Well, I can't see a UI problem when the Combine CSS is enabled while Load Dynamic Stylesheet Inline is enabled at Divi, however, while checking that we keep Divi CSS order, here is the current state/questions:

1- When Load Dynamic Stylesheet is enabled => All Divi related style sheets under the inline CSS are kept in the source (Is it expected that the sheet here is normally combined
<meta content="Divi v.4.10.7" name="generator"/><link rel='stylesheet' id='wp-block-library-css' href='https://new.rocketlabsqa.ovh/wp-includes/css/dist/block-library/style.min.css?ver=5.8.1' type='text/css' media='all' /> also it was below an inline style <style id="et-divi-open-sans-inline-css">?) => we agreed that this is fine

2- When Critical CSS is enabled while Load Dynamic Stylesheet is disabled => This file is combined <link rel='stylesheet' id='divi-style-css' href='https://new.rocketlabsqa.ovh/wp-content/themes/Divi/style.min.css?ver=4.10.7' type='text/css' media='all' /> also it was below an inline style <style id="et-divi-open-sans-inline-css"> (order changed here, so shouldn't we keep this file uncombined?) => we agreed that this is fine (combine styles that was under fonts inline)

@Tabrisrp Tabrisrp changed the title Closes combine CSS divi 4.10 compatibility Closes #4320 combine CSS divi 4.10 compatibility Oct 1, 2021
@Tabrisrp Tabrisrp added 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting type: enhancement Improvements that slightly enhance existing functionality and are fast to implement labels Oct 1, 2021
@Tabrisrp Tabrisrp added this to the 3.10.1 milestone Oct 1, 2021
Copy link
Contributor

@Tabrisrp Tabrisrp left a comment

Choose a reason for hiding this comment

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

Test & fixture should be updated based on the review's comments.

inc/ThirdParty/Themes/Divi.php Outdated Show resolved Hide resolved
inc/ThirdParty/Themes/Divi.php Outdated Show resolved Hide resolved
Copy link
Contributor

@Tabrisrp Tabrisrp left a comment

Choose a reason for hiding this comment

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

need to update the fixtures and test too

inc/ThirdParty/Themes/Divi.php Outdated Show resolved Hide resolved
Copy link

@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
testrail-report-265.pdf

Notes:
We agreed that the following is fine
1- While Critical CSS is enabled at Divi , we will combine Divi style file although it's under divi inline fonts
2- If both Load CSS and critical CSS are disabled at Divi, there will be no inline for Divi styles and we will still exclude Divi files from Combine CSS

@Mai-Saad Mai-Saad removed the needs: qa label Oct 5, 2021
@Tabrisrp Tabrisrp merged commit 2cf17ab into develop Oct 5, 2021
@Tabrisrp Tabrisrp deleted the enhancement/4320-combine-css-divi-4.10-compatibility branch October 5, 2021 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Divi 4.10+ compatibility improvement
4 participants