-
Notifications
You must be signed in to change notification settings - Fork 197
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
Show Update notice on every Sensei page #6824
Show Update notice on every Sensei page #6824
Conversation
WordPress Dependencies ReportThe
This comment was automatically generated by the |
Codecov Report
@@ Coverage Diff @@
## feature/reengaging-customers #6824 +/- ##
===============================================================
Coverage 47.27% 47.27%
- Complexity 10170 10171 +1
===============================================================
Files 500 500
Lines 36113 36115 +2
Branches 283 283
===============================================================
+ Hits 17071 17073 +2
Misses 18830 18830
Partials 212 212
Continue to review full report in Codecov by Sentry.
|
2d55b35
to
32b167e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well for me!
But I'm wondering if Sensei_Home_Notices
is really the best place to keep this feature if we display the notice on all Sensei pages. Maybe we should extract it to a more generic location?
I'm approving it anyway in case you have a strong opinion about keeping it in this class. 😉
Hey there. I totally agree with you, but, as discussed on Slack, this method has a dependency on With that said, I'm going ahead and I'll merge this PR as is. Thanks very much for your review! 😃 |
Part of https://github.com/Automattic/senseilms-com-plugins/issues/169
Proposed Changes
get_base_plugin_notice
;Testing Instructions
sensei-lms
, that is) version to refer to an old version;Pre-Merge Checklist