-
Notifications
You must be signed in to change notification settings - Fork 45
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
Closes #302 #220 #303
Closes #302 #220 #303
Conversation
@michael-milette Ready for review |
Hi @28Smiles , Thank you so much for you all your work. It is looking very good. I just came across a few minor issues that need to be addressed:
That's it. The rest is looking great. With these small changes, I will be happy to merge your suggested changes as soon as I receive them. Best regards, Michael |
Hi Leon, I was also wondering, can you think of a situation where urlencode() should be used instead of rawurlencode() as you suggested? If you can't, I think we should just update the {urlencode} tag to be RFC 3986 compliant. Thoughts? Michael |
First of all thank you for the additional pairs of eyes, I saw the error in the ci but thought it was outside of my code. Should be all fixed now.
Just wondering, will you push a release in the coming weeks, we plan on using those features with our next moodle update, to ease up the experience of our students. |
Hi Leon, Thank you very much for your contribution. I can see that you do great work and your attention to detail is excellent. Note: I amended your last commit to fix one of the examples and to give you credit in the Contributors section of the README.md file. Thanks again! Michael |
Thank you too! Leon |
Closes #302 #220
Also fixes the same bug from #302 for qrcodes