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 #6367 Add theme resolver for compatibility classes #6390

Merged
merged 13 commits into from
Feb 27, 2024

Conversation

remyperona
Copy link
Contributor

@remyperona remyperona commented Jan 17, 2024

Description

Replace current instantiation of themes compatibility classes from loading them all, to only loading the one corresponding to the current theme (if it exists).

Fixes #6367

Type of change

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

Checklists

Generic development checklist

  • My code follows the style guidelines of this project, with adapted comments and without new warnings.
  • I have added unit and integration tests that prove my fix is effective or that my feature works.
  • The CI passes locally with my changes (including unit tests, integration tests, linter).

Test summary

  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I validated all Acceptance Criteria of the related issues. (If applicable, provide proof).
  • I validated all test plan the QA Review asked me to.

@remyperona remyperona requested a review from a team January 24, 2024 22:08
@remyperona remyperona marked this pull request as ready for review January 24, 2024 22:08
@remyperona remyperona self-assigned this Jan 24, 2024
@remyperona remyperona added the type: enhancement Improvements that slightly enhance existing functionality and are fast to implement label Jan 25, 2024
@Mai-Saad
Copy link
Contributor

Mai-Saad commented Feb 14, 2024

@Tabrisrp Thanks for the PR. using the helper plugin and trying those steps 3 times, can see small improvement on the PR (instead of global time 54.80ms it is 52.71ms). However, can see UI regression in this scenario https://wpmediaqa.testrail.io/index.php?/tests/view/41471&group_by=cases:section_id&group_id=709&group_order=asc
on trunk:
trunk.webm
on PR:
branch.webm
Update
After discussion with @piotrbak and further investigation on rocketlabs site, the preview image is applied to FE. can you please check?

@MathieuLamiot
Copy link
Contributor

MathieuLamiot commented Feb 14, 2024

@Tabrisrp If I understand the old code correctly, the callback maybe_disable_youtube_preview listened to the switch_theme hook to catch when Divi was becoming the new theme. And bailed out otherwise.

Now maybe, if we are in another theme and switching to Divi, the callback is not registered since the Divi compat is not loaded so nothing happens.
On the other hand, when switching from the Divi theme, we don't bail out from the callback anymore as the condition has been removed. As a result, we apply the option restriction to the new theme.

If my understanding is correct, the issue is well described here: https://developer.wordpress.org/reference/hooks/switch_theme/
Have you considered using after_switch_theme instead?

@remyperona
Copy link
Contributor Author

Tested with after_switch_theme, it's working, updated the code to reflect

@MathieuLamiot
Copy link
Contributor

Nice!

@Mai-Saad
Copy link
Contributor

working fine now

@Mai-Saad Mai-Saad added this pull request to the merge queue Feb 27, 2024
Merged via the queue into develop with commit 83d1720 Feb 27, 2024
8 of 9 checks passed
@Mai-Saad Mai-Saad deleted the feature/theme-resolver branch February 27, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Register only the relevant 3rdParty Theme class in the 3rdPartyTheme provider
4 participants