-
Notifications
You must be signed in to change notification settings - Fork 198
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 login notice to single lesson & single quiz pages. #3299
base: trunk
Are you sure you want to change the base?
Added login notice to single lesson & single quiz pages. #3299
Conversation
@donnapep I took a swing at issue #3150 and added a login notice to both the single lesson and the single quiz pages. I updated the @hooked comments in 2 templates, added some callbacks that probably should have been mentioned there already (or not if you want to stop that practice all together), but didn't know if you want to change versions, since only the comments changed. I deleted one statement in the existing code that was there twice and was redundant. I didn't add filters on the new notices, but if you want them they are added easy enough. Let me know what you think, when you get around to it. I'll leave this one for now. |
Hi @donnapep I'll take a crack at it and will update the PR. In the code there is already a notice for the quiz page with the text "You must be logged in to take this quiz". There I can just switch the if-else to first test for user logged in and it will probably work like you propose. Do you want to make the notice text the same as your proposal for lesson? So change the text to "Please log in to take this quiz"? I also saw that the same logic should probably apply to the module page. There the enrol first notice is issued, but the situation is basically the same. And when a user should be logged in, there should be a notice saying that. Hans |
Sounds good. Consistency for the win. 🙂 I think there might even be a period missing on the existing quiz notice.
All makes sense to me. Thanks! |
Added .module-container to info-box selectors so styling of sensei messages on module pages is consistent with other sensei pages.
Don't show course signup notice when user is not logged in. In that case the user is prompted to log in first.
- Changed login_notice text to "Please log in to access {lesson|quiz}". - Used Sensei()->notices->add_notice() for quiz notices too. - Uses Sensei()->notices->add_notice() also for quiz saved message. - Probably Sensei()->frontend->messages is now redundant, but kept it just in case.
Hi @donnapep I think this is it. I made a uniform login notice for lessons, quizes and modules and it seems to work fine right now. There seems to be a conflict that prevents the merge, but that is probably something you need to look at, since I have no write access. The templates I only addes some comments to, so that can be removed and the scss I added modules to the notice-styles, since before they wheren't shown in the same styling as the notices on other sensei pages. |
@donnapep I think frontend->messages may be redundant after this pr, but that's for you to decide. I only found a save quiz message that was added that way and think it might be a legacy property. The pr uses Sensei()->notices->add_notice() instead of echo (what was used in quiz). I thought that would probably be the prefered way, since most code seems to use it at this moment. |
Now that we are starting to move away from templates and towards blocks, I've added thoughts on a block-based solution. |
Fixes #3150
Changes proposed in this Pull Request