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

Added Notifications tab to BuddyPress integration. #693

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@yojance
Contributor

yojance commented Oct 16, 2018

Description

Added Notifications tab to BuddyPress integration.

Fixes #627

How has this been tested?

Code has been tested locally and it works as expected. It's using the same template from myaccount/my-notifications.php

Screenshots

lifterlms-buddypress-notifications-tab

Todo

The pagination will not keep the user in the same page. Clicking on Next will take them to my-courses/notifications/?sdpage=2 instead of /members/yojance/courses/notifications/?sdpage=2. I'm open to suggestions if you'd like this behavior changed.

Checklist:

  • My code has been tested.
@thomasplevy

This comment has been minimized.

Member

thomasplevy commented Oct 16, 2018

@yojance looks pretty solid but we can probably prevent a disjointed user experience related to your TODO items with the following updates:

  • Add an apply_filter on the $url variable in the LLMS_Student_Dashboard::output_notifications_content() function
  • Use an add_filter to modify the default URL in the BP function added to this PR and the remove_filter immediately after output

This shouldn't be too much additional code or overhead and will prevent users from being bumped out of their BP profile.

Do you want to take a stab at that an update the PR?

Thanks for the effort thus far regardless

@thomasplevy

This comment has been minimized.

Member

thomasplevy commented Oct 16, 2018

Travis fail is acceptable. Timestamp delta issue on PHP 7.0 only.

@thomasplevy

This comment has been minimized.

Member

thomasplevy commented Oct 16, 2018

Woops, didn't mean to close

@thomasplevy thomasplevy reopened this Oct 16, 2018

'slug' => 'notifications',
'parent_slug' => 'courses',
'parent_url' => $parent_url,
'screen_function' => array( $this,'notifications_screen' ),

This comment has been minimized.

@thomasplevy

thomasplevy Oct 16, 2018

Member

code climate identified a code styling error here, there should be a space between the comma and single quote on this line (105)

@thomasplevy

Please see comments below on code styling issues and update accordingly.

These CS errors can be quickly resolved with phpcbf: https://github.com/gocodebox/lifterlms#installing-for-development

@yojance

This comment has been minimized.

Contributor

yojance commented Oct 17, 2018

@thomasplevy

I'm not 100% sure on how to implement your suggestions. I've taken it as far as I could but I can't seem to get it to work. I believe I've seen the technique you are suggesting but I just can't get it to work. Help!

@thomasplevy

This comment has been minimized.

Member

thomasplevy commented Oct 17, 2018

@yojance your code should be working but I'm not certain why it's not. I'll take a look at it and see what I can find

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment