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 Course List block to My Courses on page creation #6343

Merged
merged 30 commits into from Jan 19, 2023

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Dec 28, 2022

Implements #6341

Changes proposed in this Pull Request

  • Make sure you have an existing installation of Sensei having this branch checked out; keep it as it is
  • Create a fully new local WP site containing this branch
  • Install the Sensei Plugin in the new installation and complete the onboarding
  • Create multiple Courses and publish them
  • Create a student user
  • Login as the user, go to my courses page
  • Make sure you see the course list block instead of the Student Courses block
  • Make sure the filter and paginations are working
  • Now go to the wp installation with Sensei that you already had
  • Go to the My Courses page
  • Make sure it appears as before, with Student Courses block

@Imran92 Imran92 requested a review from a team December 28, 2022 22:14
@Imran92 Imran92 self-assigned this Dec 28, 2022
@codecov
Copy link

codecov bot commented Dec 28, 2022

Codecov Report

Merging #6343 (3ddc975) into trunk (086650c) will increase coverage by 0.12%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #6343      +/-   ##
============================================
+ Coverage     45.97%   46.10%   +0.12%     
- Complexity     9551     9564      +13     
============================================
  Files           459      460       +1     
  Lines         33837    33886      +49     
  Branches        275      275              
============================================
+ Hits          15558    15623      +65     
+ Misses        18074    18058      -16     
  Partials        205      205              
Impacted Files Coverage Δ
assets/blocks/course-list-block/index.js 38.77% <ø> (ø)
...cks/course-list/class-sensei-course-list-block.php 16.66% <16.66%> (ø)
...rse-list/class-sensei-course-list-filter-block.php 89.65% <88.23%> (-1.46%) ⬇️
...ourse-list-filter-block/course-list-filter-edit.js 57.14% <100.00%> (ø)
includes/admin/class-sensei-setup-wizard-pages.php 97.67% <100.00%> (+74.59%) ⬆️
includes/blocks/class-sensei-global-blocks.php 100.00% <100.00%> (ø)
...ist/class-sensei-course-list-categories-filter.php 94.28% <100.00%> (+0.53%) ⬆️
...-list/class-sensei-course-list-featured-filter.php 93.33% <100.00%> (+0.74%) ⬆️
...-list/class-sensei-course-list-filter-abstract.php 100.00% <100.00%> (ø)
...class-sensei-course-list-student-course-filter.php 93.87% <100.00%> (+0.39%) ⬆️
... and 2 more

Continue to review full report at Codecov.

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

@m1r0
Copy link
Member

m1r0 commented Jan 2, 2023

Hey Imran, happy new year! 🎆 🥂 🤝 🙂

I'm not sure if this is related to your PR, but I've noticed some issues with the Storefront theme. I haven't tested on many other themes, but it might be a good idea.

image

Another thing I've noticed is that the login form is no longer accessible. If you are not logged in and go to My Courses, you see a list of all courses. If you try to enroll, you get redirected back to My Courses but you don't see the login form. Do you think it makes sense to show the login form on the My Courses page as before?

@Imran92
Copy link
Contributor Author

Imran92 commented Jan 5, 2023

Thanks a lot for checking @m1r0 ❤️ I've updated several parts of the PR, can you kindly give it a check again? Thanks!

@donnapep
Copy link
Collaborator

donnapep commented Jan 5, 2023

I left some comments on the P2 post re: the login form.

@Imran92
Copy link
Contributor Author

Imran92 commented Jan 9, 2023

I left some comments on the P2 post re: the login form.

Thanks a lot for the comments! Updated as per our discussions there here

Copy link
Member

@m1r0 m1r0 left a comment

Choose a reason for hiding this comment

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

Hey, I've tested the login form and it works as described. Nice! 🎉 Left one really minor suggestion.

But my main concern here is that I feel this block still doesn't look that good on most themes:

Astra
image

Divi
image

Hello Elementor
image

Maybe we should polish it a bit before making it the default?

cc @donnapep

@m1r0
Copy link
Member

m1r0 commented Jan 10, 2023

Are the E2E tests failing related to this PR?

@donnapep
Copy link
Collaborator

Maybe we should polish it a bit before making it the default?

Yes, I think so. I wouldn't worry about Hello Elementor, but it should definitely look good on Course, Divi and Astra.

@github-actions
Copy link

github-actions bot commented Jan 11, 2023

WordPress Dependencies Report

The github-action-wordpress-dependencies-report action has detected some script changes between the commit 3ddc975 and trunk. Please review and confirm the following are correct before merging.

Script Handle Added Dependencies Removed Dependencies Total Size Size Diff
blocks/global-blocks.js 20.3 kB +75 B ( +0.38% 🔼 )
blocks/course-list-filter-block/course-list-filter.js 246 B +50 B ( +25.52% 🔼 )
blocks/shared.js 13.8 kB +2 B ( +0.02% 🔼 )

This comment was automatically generated by the github-action-wordpress-dependencies-report action.

@Imran92
Copy link
Contributor Author

Imran92 commented Jan 11, 2023

Thanks for taking a look @m1r0 and @donnapep <3 I've updated them a bit, and now hopefully it'll look decent across themes. Here are some screenshots from different themes

Course
Screen Shot 2023-01-12 at 3 27 49 AM

Astra
Screen Shot 2023-01-12 at 3 47 29 AM

Divi
Screen Shot 2023-01-12 at 3 30 21 AM

@donnapep
Copy link
Collaborator

I did some testing on the Course theme and here's what I found:

  • My Courses displays pagination and shows 3 courses at a time, even though I've set Settings > Reading > Blog pages show at most = 10. Even if I delete the page, change that value to 2, and run the wizard again, it still shows 3 posts. This doesn't happen on the archive page, which works great with that setting.
  • When filtering My Courses page and I'm on page 2, I don't see any results. Instead, I see a blank page. Maybe we should redirect back to page 1 after filtering? For course archive, I get a 404. (Feel free to log this one as a separate issue and address it later if this is an existing issue with the Course List Filter block.)

@Imran92
Copy link
Contributor Author

Imran92 commented Jan 13, 2023

Thanks for taking a look @donnapep !

My Courses displays pagination and shows 3 courses at a time, even though I've set Settings > Reading > Blog pages show at most = 10. Even if I delete the page, change that value to 2, and run the wizard again, it still shows 3 posts. This doesn't happen on the archive page, which works great with that setting.

My Courses page probably doesn't consider this setting, I tried with Student Courses block as it had before and it didn't consider the number of posts set in the reader. That setting works with Archive pages global query is used there. I've just checked that the Query Loop block does not consider this setting either. So we'll have to find a way to make it work if we want Query Loop to support it. As this is a new behavior, do you think we should create a separate ticket for this and try to implement it there? And if we do, should that be one with priority for our release?

When filtering My Courses page and I'm on page 2, I don't see any results. Instead, I see a blank page. Maybe we should redirect back to page 1 after filtering? For course archive, I get a 404. (Feel free to log this one as a separate issue and address it later if this is an existing issue with the Course List Filter block.)

Very good idea, sounds like a nice improvement for the Course List Filter block. I'll create a ticket for it and implement it under that. Thanks you!

@Imran92
Copy link
Contributor Author

Imran92 commented Jan 17, 2023

Thanks a lot for reviewing @donnapep !

Although the Active filter is set by default, when I view My Courses it's showing all courses, not just the active ones.

I've updated the code here and here, now the frontend will filter considering the default value.

If I have a paid course on the second page and I click the Buy Course button, I don't see the notification that the course was added to the cart.

This issue was due to the Course Purchase button removing all query params from URL, it was the user to the first page every time and as the added course wasn't there, the notice wasn't shown. I've fixed it here in sensei-pro.

we're setting per_page to 3 here. It seems to me as though it would be better to show all courses on the same page. I think grid view may be setting a limit as well. Perhaps we should show however many courses the legacy My Courses page shows?

I've updated the page pattern here to show the same number of courses as the legacy block.

As for post per page, Query Loop block by default does not allow showing all courses, even if we provide no value for perPage, it'll still add a limit. One way around this can be setting the perPage value very high, like 1000, so that it becomes unlikely to go past that. Should it be alright if we do that?

The limit with Grid comes from the default grid pattern we created. We can increase it if we want. The catch will be users adding the pattern anywhere will now have that increased number. We can do it too if that's okay.

When filtering My Courses page and I'm on page 2, I don't see any results. Instead, I see a blank page. Maybe we should redirect back to page 1 after filtering

Updated it here, now when filtering, we'll be taken to the first page. I'll make it work for Archive too in the Archive PR.

@donnapep
Copy link
Collaborator

As for post per page, Query Loop block by default does not allow showing all courses, even if we provide no value for perPage, it'll still add a limit. One way around this can be setting the perPage value very high, like 1000, so that it becomes unlikely to go past that. Should it be alright if we do that?

Showing 10 for the list view and 12 for the grid view as it works now looks good, so we can leave it as is. 👍🏻

donnapep
donnapep previously approved these changes Jan 17, 2023
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! Left a few minor comments.

@Imran92
Copy link
Contributor Author

Imran92 commented Jan 19, 2023

Updated the hook name as per the conversation here

@Imran92 Imran92 added the Hooks This change adds or modifies one or more hooks. label Jan 19, 2023
@donnapep
Copy link
Collaborator

I get the same fatal error as on the course archive page when I don't select a pattern, but I assume this fix will resolve it?

@Imran92
Copy link
Contributor Author

Imran92 commented Jan 19, 2023

I get the same fatal error as on the course archive page when I don't select a pattern, but I assume this fix will resolve it?

Yeah it'll fix this one too

@Imran92 Imran92 removed the Hooks This change adds or modifies one or more hooks. label Jan 19, 2023
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.

🥳

@Imran92 Imran92 merged commit 7d9c5bc into trunk Jan 19, 2023
@Imran92 Imran92 deleted the add/course-list-block-in-my-courses-page branch January 19, 2023 15:56
@donnapep donnapep added this to the 4.11 milestone Jan 19, 2023
@donnapep donnapep changed the title Add course list block in my courses page Add Course List block to My Courses on page creation Jan 19, 2023
@merkushin merkushin mentioned this pull request Jan 26, 2023
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.

Replace Student Courses block in My Courses page with Course List block
3 participants