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

Fix duplicate comment section under lesson in FSE themes #7044

Merged
merged 8 commits into from
Jul 28, 2023

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Jul 25, 2023

Resolves #7037

Proposed Changes

In FSE themes, like Course, Twenty Twenty Three etc, we were seeing two comment sections. After some investigation, we understood that one of them (the one at the bottom) was coming from the page template of those themes (It also turned out it's not using the right template, so we created another issue for it). So this was the expected one, but the top one was unexpected.

We found that there are some custom processes in place for lesson post type, there we manually call the comments_template function on a hook. But as the deprecation hook messages suggest in linked code, that there is no comments.php template available for the theme, which is expected. But it was still echoing comments. Looking deeper, we found out that it uses a fallback default template from the compat folder in core even if it doesn't find any template in the theme, as it comes from core, they don't follow any design from our block theme's comment section. So we made calling the function conditional for non FSE themes.

Doing the above was rendering single comment section only from the FSE theme's page template as expected, but the comment reply form was still not loading. Digging into that, we found that for all unsupported themes (every block theme), we're disabling comments using a hook here and here. So we made them conditional for FSE as well.

Testing Instructions

  1. Disable Learning Mode in Sensei LMS -> Settings -> Appearance.
  2. Create a lesson
  3. In that lesson's setting section from the right side, enable comments
  4. Take the lesson as a student
  5. Try commenting and replying to comments.
  6. Make sure you see only one comment section and that it's working as expected.

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly documented
  • Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • All strings are translatable (without concatenation, handles plurals)
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • Hooks (p6rkRX-1uS-p2) and functions are documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate
  • Code is tested on the minimum supported PHP and WordPress versions
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
  • "Needs Documentation" label is added if this change requires updates to documentation
  • Known issues are created as new GitHub issues

@Imran92 Imran92 added this to the 4.16.1 milestone Jul 25, 2023
@Imran92 Imran92 requested a review from a team July 25, 2023 10:18
@Imran92 Imran92 self-assigned this Jul 25, 2023
@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Merging #7044 (771025e) into trunk (f243b2c) will increase coverage by 0.00%.
Report is 1 commits behind head on trunk.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              trunk    #7044   +/-   ##
=========================================
  Coverage     48.02%   48.03%           
- Complexity    10458    10465    +7     
=========================================
  Files           573      573           
  Lines         44145    44151    +6     
  Branches        402      402           
=========================================
+ Hits          21201    21207    +6     
  Misses        22617    22617           
  Partials        327      327           
Files Changed Coverage Δ
includes/class-sensei-lesson.php 35.68% <100.00%> (ø)
includes/class-sensei-utils.php 52.44% <100.00%> (+0.08%) ⬆️
...ers/class-sensei-unsupported-theme-handler-cpt.php 78.68% <100.00%> (+1.49%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29c898b...771025e. Read the comment docs.

@donnapep
Copy link
Collaborator

Wow, excellent detective work! 🔍 We should also test on unsupported themes to ensure comments still behave as before, correct?

@merkushin
Copy link
Member

We should also test on unsupported themes to ensure comments still behave as before, correct?

Looks like that as the call is now conditional. Do you know an example of an unsupported theme? :)

@donnapep
Copy link
Collaborator

Do you know an example of an unsupported theme? :)

Yup, any theme for which we don't provide a custom wrapper and that doesn't explicitly declare Sensei support (e.g. Divi and Astra). More details here.

Copy link
Member

@merkushin merkushin left a comment

Choose a reason for hiding this comment

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

Great investigation!

Left a few suggestions for tests.

tests/unit-tests/test-class-utils.php Outdated Show resolved Hide resolved
tests/unit-tests/test-class-utils.php Show resolved Hide resolved
tests/unit-tests/test-class-utils.php Outdated Show resolved Hide resolved
tests/unit-tests/test-class-utils.php Outdated Show resolved Hide resolved
@merkushin
Copy link
Member

Following test instructions (I added a 0-item there, by the way),

with the Course theme: works well (the spacing there is a bit weird, but probably it is out of scope of this card).
CleanShot 2023-07-27 at 00 35 09@2x

Astra works well, but here we can see issues with styles as well:
CleanShot 2023-07-27 at 00 34 44@2x

Divi:
CleanShot 2023-07-27 at 00 34 16@2x

@Imran92
Copy link
Contributor Author

Imran92 commented Jul 26, 2023

Thanks for the review @merkushin !

I've updated the test names and have split the act and assertions here b35c319.

Following test instructions (I added a 0-item there, by the way),

Also looks like it's working as expected for Course, Divi and Astra, so it's working in our popular themes. I've also checked using them. And as you've said, design of the comment section is not in the scope of this PR. But for the Course theme, we're fixing all the comment stylings under this card Automattic/themes#6977.

Please do take a look again when possible. Thanks!

@Imran92
Copy link
Contributor Author

Imran92 commented Jul 26, 2023

Wow, excellent detective work! 🔍 We should also test on unsupported themes to ensure comments still behave as before, correct?

Thanks @donnapep ! <3 It was fun! Yap, we should check. I checked with our top popular themes to see if it's working as expected.

Copy link
Collaborator

@donnapep donnapep left a comment

Choose a reason for hiding this comment

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

Looks good from my perspective, but I'll leave final approval to @merkushin to ensure his comments are addressed to his satisfaction. 🙂

Copy link
Member

@merkushin merkushin left a comment

Choose a reason for hiding this comment

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

Thanks Imran! Checked with the updated course theme, looks good :)

@Imran92 Imran92 merged commit c08e2cd into trunk Jul 28, 2023
20 checks passed
@Imran92 Imran92 deleted the fix/duplicate-comment-section-in-fse-themes branch July 28, 2023 12:30
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.

Fix the issue of two comment sections getting rendered under lessons in all FSE themes
3 participants