-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
[notifications-docs] Restructured Documentation: New Unified Documentation Website #282
Conversation
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.
Coming to user/sending-notifications.rst
from a link in the Controller docs, I noticed some area for improvements there.
docs/user/sending-notifications.rst
Outdated
accepted parameters from `django-notifications documentation | ||
<https://github.com/django-notifications/django-notifications#generating-notifications>`_. | ||
|
||
Additional ``notify`` Keyword Arguments |
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.
Why do we have an additional section for these parameters? Why can't this section be merged with the previous one?
docs/user/sending-notifications.rst
Outdated
@@ -78,7 +77,7 @@ Additional ``notify`` Keyword Arguments | |||
================= ====================================================== | |||
|
|||
Passing Extra Data to Notifications | |||
----------------------------------- | |||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
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.
What do you think about adding a call to notify()
in the following example to make it clearer?
docs/user/sending-notifications.rst
Outdated
@@ -18,7 +18,7 @@ Notifications can be created using the ``notify`` signal. Eg: | |||
notify.send( | |||
sender=admin, | |||
recipient=operators, | |||
type='default', | |||
type="default", | |||
description="Test Notification", | |||
verb="Test Notification", | |||
email_subject="Test Email Subject", |
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.
are you sure this example makes perfect sense?
I was thinking we could use the generic message notification as an example which allows sending a custom message, while the example after register_notification_type
would show how to use the typical case of sending a notification of a type which has already been defined and we don't need to pass any message or description because it's been already defined in the type. I would like this important concept to be more easier to grasp in this document, I don't think it's clearly explained now.
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.
Ready!
Related to openwisp/openwisp-docs#107.
These documents are pulled by the logic being worked on in openwisp/openwisp-docs#189.
We are doing similar changes in all modules.