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 breadcrumbs to templates #2491

Merged
merged 15 commits into from
Jun 6, 2024
Merged

Add breadcrumbs to templates #2491

merged 15 commits into from
Jun 6, 2024

Conversation

renintw
Copy link
Contributor

@renintw renintw commented Jun 3, 2024

Add breadcrumbs to the sites, except for Home and the Learning Pathway taxonomy. See design.

Closes #2442

I noticed in the design there's a Learn WordPress breadcrumb, which link does it link to? It appears to be redundant with 'Home'. @WordPress/meta-design
image

Screenshots

post type screenshot
Home Screenshot 2024-06-04 at 00 17 08
Post Screenshot 2024-06-04 at 00 56 43
My Course Screenshot 2024-06-04 at 00 27 47
Online Workshops Screenshot 2024-06-04 at 00 27 23
Apply to Faciliate Screenshot 2024-06-04 at 00 27 30
Grandchild Page Screenshot 2024-06-04 at 00 27 39
Contribute Screenshot 2024-06-04 at 00 27 13
Lessons Archive Screenshot 2024-06-04 at 00 27 00
Courses Archive Screenshot 2024-06-04 at 00 17 32
Single Course Screenshot 2024-06-04 at 00 17 45
Learning Pathways (got some issues rendering image here in the local env, but yeah this is the learning pathways) Screenshot 2024-06-04 at 00 17 20

@renintw renintw added [Component] Learn Theme Website development issues related to the Learn theme. Dev labels Jun 3, 2024
@renintw renintw requested a review from adamwoodnz June 3, 2024 16:27
@renintw renintw self-assigned this Jun 3, 2024
@adamwoodnz
Copy link
Contributor

Learning Pathways (got some issues rendering image here in the local env, but yeah this is the learning pathways)

Looks like this is actually the section title. $content should be coming from this function, seems like the reference is not working for you?

@adamwoodnz
Copy link
Contributor

Single lesson is showing the course name but not the lesson name

Screenshot 2024-06-04 at 11 02 21 AM

Copy link
Contributor

@adamwoodnz adamwoodnz left a comment

Choose a reason for hiding this comment

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

Looking good! Apart from the second 'Learn WordPress' breadcrumb you've noted, the only other thing I've noticed is that the /learning-pathways page shouldn't have breadcrumbs

Screenshot 2024-06-04 at 11 07 02 AM

@fcoveram
Copy link

fcoveram commented Jun 4, 2024

I noticed in the design there's a Learn WordPress breadcrumb, which link does it link to? It appears to be redundant with 'Home'.

You are right @renintw. The "Learn WordPress" link should not be there, sorry for the confusion. The mockups were updated.

@renintw
Copy link
Contributor Author

renintw commented Jun 4, 2024

Looks like this is actually the section title. $content should be coming from this function, seems like the reference is not working for you?

Since I got all these error messages, those lines are currently commented out.
What are the steps to properly set up the learning pathway config? I reckon some settings need to be done in the wp-admin. And does it require to activate sensei pro to make the functions fully work?

image

Notice: Trying to get property ‘term_id’ of non-object in /var/www/html/wp-content/themes/pub/wporg-learn-2024/patterns/taxonomy-learning-pathway-content.php on line 11 Notice: Trying to get property ‘slug’ of non-object in /var/www/html/wp-content/themes/pub/wporg-learn-2024/patterns/taxonomy-learning-pathway-content.php on line 12 Notice: Trying to get property ‘term_id’ of non-object in /var/www/html/wp-content/themes/pub/wporg-learn-2024/patterns/taxonomy-learning-pathway-content.php on line 14 Notice: Trying to get property ‘term_id’ of non-object in /var/www/html/wp-content/themes/pub/wporg-learn-2024/patterns/taxonomy-learning-pathway-content.php on line 15 Notice: Trying to get property ‘term_id’ of non-object in /var/www/html/wp-content/themes/pub/wporg-learn-2024/patterns/taxonomy-learning-pathway-content.php on line 16 Notice: Undefined index: in /var/www/html/wp-content/themes/pub/wporg-learn-2024/functions.php on line 214

Notice: Trying to access array offset on value of type null in /var/www/html/wp-content/themes/pub/wporg-learn-2024/patterns/taxonomy-learning-pathway-content.php on line 51
Notice: Trying to access array offset on value of type null in /var/www/html/wp-content/themes/pub/wporg-learn-2024/patterns/taxonomy-learning-pathway-content.php on line 58```

@renintw
Copy link
Contributor Author

renintw commented Jun 4, 2024

the only other thing I've noticed is that the /learning-pathways page shouldn't have breadcrumbs

Missed that page, there are currently no CTAs that can link to this page right?
Fix: 55c8358

Screenshot 2024-06-05 at 04 26 10

@renintw
Copy link
Contributor Author

renintw commented Jun 4, 2024

Single lesson is showing the course name but not the lesson name

A lesson that belongs to a course can be accessed from the lesson archive or from its associated course page. If accessed from the course page, the breadcrumbs shown in the screenshot below would make sense. However, if accessed from the lesson archive, the user might expect to be able to return to the lesson archive?
Currently, I am providing a more consistent experience for users who enter a lesson from its associated course.
What do you think? (I'm thinking if users want to go back to the lesson archive, they can simply use the back button. Moreover, when they click into a lesson and see that it belongs to a specific course, it can potentially encourage them to explore more content. It's still a bit inconsistent in a sense, though)

screenshot
lesson not belonging to any course Screenshot 2024-06-05 at 04 11 57
lesson belonging to certain course Screenshot 2024-06-05 at 04 34 14

@renintw renintw requested a review from adamwoodnz June 4, 2024 19:38
@adamwoodnz
Copy link
Contributor

Single lesson is showing the course name but not the lesson name

A lesson that belongs to a course can be accessed from the lesson archive or from its associated course page. If accessed from the course page, the breadcrumbs shown in the screenshot below would make sense. However, if accessed from the lesson archive, the user might expect to be able to return to the lesson archive? Currently, I am providing a more consistent experience for users who enter a lesson from its associated course. What do you think? (I'm thinking if users want to go back to the lesson archive, they can simply use the back button. Moreover, when they click into a lesson and see that it belongs to a specific course, it can potentially encourage them to explore more content. It's still a bit inconsistent in a sense, though)

screenshot
lesson not belonging to any course Screenshot 2024-06-05 at 04 11 57
lesson belonging to certain course Screenshot 2024-06-05 at 04 34 14

I think it would be quite unusual for a Lesson to not be part of a Course, so I think prioritising that structure makes sense. In the rare case when it is free standing, having the lesson archive as the parent makes sense to me.

@adamwoodnz
Copy link
Contributor

Looks like this is actually the section title. $content should be coming from this function, seems like the reference is not working for you?

Since I got all these error messages, those lines are currently commented out. What are the steps to properly set up the learning pathway config? I reckon some settings need to be done in the wp-admin. And does it require to activate sensei pro to make the functions fully work?

This isn't using any sensei pro features, just a custom taxonomy. Maybe you don't have the taxonomy set up with the expected slugs; there should be developer, user, designer, and contributor. You can edit the slugs at http://localhost:8888/wp-admin/edit-tags.php?taxonomy=learning-pathway&post_type=course

Screenshot 2024-06-05 at 8 54 01 AM

I've added a taxonomy guard in this PR but obviously needs more to check the term exists.

@adamwoodnz
Copy link
Contributor

Also just noticed there should be no breadcrumbs on the Courses or Lessons archives, sorry I missed this, maybe the designs changed...

@adamwoodnz
Copy link
Contributor

And the empty breadcrumbs are still taking up space on the pages where we return early:

Learning Pathways Learning Pathway
Screenshot 2024-06-05 at 1 56 06 PM Screenshot 2024-06-05 at 1 55 17 PM

@renintw
Copy link
Contributor Author

renintw commented Jun 5, 2024

Also just noticed there should be no breadcrumbs on the Courses or Lessons archives, sorry I missed this, maybe the designs changed...

Just to double confirm, are those pages (archives) supposed to not have breadcrumbs? @WordPress/meta-design Thanks.

@renintw
Copy link
Contributor Author

renintw commented Jun 5, 2024

I think it would be quite unusual for a Lesson to not be part of a Course, so I think prioritising that structure makes sense. In the rare case when it is free standing, having the lesson archive as the parent makes sense to me.

ah. I mean for lessons that are part of a Course, users can typically get into those lessons in two ways:

  1. from a course.
  2. from the homepage or lesson archive.

The current breadcrumbs implementation makes the flow consistent for users entering a lesson through a course page. (they can see the previous paths, which include the course title and the course archive)
but it kind of isn't when users coming from the homepage or lesson archive. (I imagine users would expect to see the Learn archive as the previous path in the breadcrumbs)

@renintw
Copy link
Contributor Author

renintw commented Jun 5, 2024

Maybe you don't have the taxonomy set up with the expected slugs; there should be developer, user, designer, and contributor.

I had set these values from the beginning. I just added the missing contributor, but it's still the same.

Update

Fixed.

@fcoveram
Copy link

fcoveram commented Jun 5, 2024

Also just noticed there should be no breadcrumbs on the Courses or Lessons archives, sorry I missed this, maybe the designs changed...

Just to double confirm, are those pages (archives) supposed to not have breadcrumbs?

Exactly. No breadcrumb on pages that are visible in the local nav. That includes Courses and Pages, and from these screenshots I see it's also on My courses. It should be hidden on this page as well.

Regarding landing on the "Learning Pathway" page (as per this comment), it was intentionally not linked from the homepage and local nav. Adding an index page that does not provide more info for browsing content felt unnecessary.

@adamwoodnz
Copy link
Contributor

I think it would be quite unusual for a Lesson to not be part of a Course, so I think prioritising that structure makes sense. In the rare case when it is free standing, having the lesson archive as the parent makes sense to me.

ah. I mean for lessons that are part of a Course, users can typically get into those lessons in two ways:

  1. from a course.
  2. from the homepage or lesson archive.

The current breadcrumbs implementation makes the flow consistent for users entering a lesson through a course page. (they can see the previous paths, which include the course title and the course archive) but it kind of isn't when users coming from the homepage or lesson archive. (I imagine users would expect to see the Learn archive as the previous path in the breadcrumbs)

The breadcrumbs aren't supposed to reflect the browsing history though, rather the information hierarchy. That's what I expect, if I want to go back I can use browser history.

@adamwoodnz
Copy link
Contributor

Maybe you don't have the taxonomy set up with the expected slugs; there should be developer, user, designer, and contributor.

I had set these values from the beginning. I just added the missing contributor, but it's still the same.

Hmm I'm not sure what's going on then. It isn't an issue on staging or prod, so it seems likely it's local config. Happy to help debug if you like.

@renintw
Copy link
Contributor Author

renintw commented Jun 5, 2024

The feedback mentioned above has already been addressed.

@fcoveram There are still some inconsistencies though. I'd like to confirm a few things:

  1. The local nav only goes up to the second level, right?
    Like Learn WordPress。Courses (First 。Second)
    image

  2. Pages should not have breadcrumbs, but I see some do and some don't in the design.

  3. If pages shouldn't have breadcrumbs, should a child or grandchild have them? I see it does in the design. (And what do you think the local nav should look like in this case?)
    image

Does it make sense to summarize that the local navigation will display up to the second level, and breadcrumbs start to display from when it's the third level?
It looks like this is the case for the developer pages (although its local nav doesn't display the second level directly like this design unless scrolled)

Copy link
Contributor

@adamwoodnz adamwoodnz left a comment

Choose a reason for hiding this comment

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

Code changes LGTM. You could ship this then iterate on the page things in a separate PR maybe.

@fcoveram
Copy link

fcoveram commented Jun 6, 2024

I think we're missing the part of how local nav and breadcrumb behave. IIRC, all discussion and decision happened in wporg-mu-plugins#535. Here are the designs that show different uses cases across the site, and the diagram below summarizes what's in the design file.

Diagram of local nav behavior

In this screenshot, Learn WordPress is a link to self and Course is a label of the page where users are that show up when scrolling down. Take a look at the accessibility page.

@renintw
Copy link
Contributor Author

renintw commented Jun 6, 2024

Thanks @fcoveram, that's very clear!
I'll move the tweaks of the local nav along with the breadcrumbs of pages into another ticket.

@renintw renintw merged commit 052f794 into trunk Jun 6, 2024
1 check passed
@renintw renintw deleted the 2442-add-breadcrumbs branch June 6, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Component] Learn Theme Website development issues related to the Learn theme. Dev
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add breadcrumbs to templates
3 participants