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

Add "sensei" CSS class to all Sensei LMS pages #3275

Conversation

hansschuijff
Copy link
Contributor

@hansschuijff hansschuijff commented Jun 6, 2020

As a result of this PR the sensei body class will automatically be added to:

  1. Course results pages
  2. Learners profiles
  3. Teacher archive pages

Fixes #3193

Changes proposed in this Pull Request

  • Add "sensei" CSS body class on relevant pages that are missing it.

Testing instructions

Impacted pages:

  • /course/{course-slug}/results/
  • /learner/{username}/
  • /author/{author}/

As a result the sensei body class will automatically be added to:
1. Course results pages
2. Learners profiles
3. Teacher archive pages
@hansschuijff hansschuijff changed the title Fix #3193: Add missing pages to is_sensei() Fixes #3193: Add missing pages to is_sensei() Jun 6, 2020
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.

I think we should move the new helper functions to the Sensei_Utils class to keep them out of the global namespace.

Also, the modules page is still missing the sensei CSS class.

includes/sensei-functions.php Outdated Show resolved Hide resolved
includes/sensei-functions.php Outdated Show resolved Hide resolved
includes/sensei-functions.php Outdated Show resolved Hide resolved
includes/sensei-functions.php Outdated Show resolved Hide resolved
includes/sensei-functions.php Outdated Show resolved Hide resolved
includes/sensei-functions.php Outdated Show resolved Hide resolved
includes/sensei-functions.php Outdated Show resolved Hide resolved
includes/sensei-functions.php Outdated Show resolved Hide resolved
includes/sensei-functions.php Outdated Show resolved Hide resolved
includes/sensei-functions.php Outdated Show resolved Hide resolved
@donnapep
Copy link
Collaborator

donnapep commented Jun 15, 2020

Instead of having separate elseif statements for each branch that does the same thing and sets $is_sensei to true, it would be cleaner to combine them into a single if:

if ( in_array( $post->ID, array( $course_page_id, $my_courses_page_id ) )
	|| sensei_is_learner_profile()
	|| sensei_is_course_results_page()
	|| sensei_is_teacher_archive()
) {
	$is_sensei = true;
}

@hansschuijff
Copy link
Contributor Author

Also, the modules page is still missing the sensei CSS class.

@donnapep You just found a bug in version 3.1.1. It is not caused by this PR. When I open a module page I get two errors:

Notice:  Trying to get property 'labels' of non-object in \wp-includes\general-template.php on line 3030
Notice:  Trying to get property 'singular_name' of non-object in wp-includes\general-template.php on line 3030

Looking at what happens there it gets the term and tries to get the tax using $term->taxonomy, but that property is not defined so get_taxonomy( $term->taxonomy ) returns false instead of a taxonomy object causing these notices. I haven't why this error occurs, but it occurs in the released version 3.1.1 too.

@donnapep
Copy link
Collaborator

Looking at what happens there it gets the term and tries to get the tax using $term->taxonomy, but that property is not defined so get_taxonomy( $term->taxonomy ) returns false instead of a taxonomy object causing these notices. I haven't why this error occurs, but it occurs in the released version 3.1.1 too.

Could this also have something to do with the Genesis framework? I don't recall ever seeing a notice on the module page with any theme I use. Perhaps for the purposes of this PR you could trying testing with a non-Genesis theme to get the modules page working?

@hansschuijff
Copy link
Contributor Author

hansschuijff commented Jun 15, 2020

Could this also have something to do with the Genesis framework?

@donnapep No I'm alert on that now. I'm testing with a newly installed unaltered twenty twenty theme at the moment. I tested with previous version and had to go back to version 2.4.0 to get rid of the notices. In version 3.0.0 upwards get_queried_object(); returns a WP_Post object instead of a WP_Term object and that leads to the notices.

However when I tried the Storefront theme, since it has theme support by default, I didn't get the notices there. There a WP_Term object is returned by get_queried_object(); and the sensei class is added to the module page like expected. Could it be that the error only occurs in compatibility mode?

@donnapep
Copy link
Collaborator

OK, I can reproduce with Divi. Regardless though, that's a separate issue that we shouldn't try to fix as part of this PR as PRs should be focused on fixing one thing.

@donnapep
Copy link
Collaborator

Actually, looks like there's already an issue for that. For this one, doesn't look like we need to worry about the modules page then. After the requested changes are made, this one will be ready.

@hansschuijff
Copy link
Contributor Author

@donnapep I just pushed the update. Should be alright now. Otherwise let me know.

@donnapep donnapep added this to the 3.2.0 milestone Jun 16, 2020
@donnapep donnapep changed the title Fixes #3193: Add missing pages to is_sensei() Add "sensei" CSS class to all Sensei LMS pages Jun 16, 2020
@donnapep donnapep merged commit c692520 into Automattic:master Jun 16, 2020
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.

Add "sensei" CSS body class on relevant pages that are missing it
2 participants