-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
WIP: Notification profile #5824
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.
There are accessibility issues in these changes.
@@ -69,6 +72,11 @@ <h3> {% if scope == 'system' %}System{% else %}Personal{% endif %} Notification | |||
<td class="centered"> | |||
<input type="checkbox" name="{{ field.name }}" {% if c in field.value %}checked{% endif %} value="{{ c }}"/> | |||
</td> | |||
{% elif scope == 'template' and c in enabled_notifications and c != 'msteams' %} | |||
<td class="centered"> | |||
<input type="checkbox" name="{{ field.name }}" {% if c in field.value %}checked{% endif %} value="{{ c }}"/> |
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.
Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.
fd234ba
to
d153b46
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.
Just a small comment, the rest is great.
e0e6a61
to
76b0803
Compare
Conflicts have been resolved. A maintainer will review the pull request shortly. |
657c5f2
to
a5cd230
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Conflicts have been resolved. A maintainer will review the pull request shortly. |
@dsever Is this PR still a draft or ready for review? |
@StefanFl I think it is ready. |
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.
Looks pretty solid to me
I will have a look at it as soon as possible |
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.
Looks good to me, great work @dsever
Screencast.2022-01-27.22.09.24.mp4
It implements single template for notifications that is applied automatically during the user creation (GUI only)
@StefanFl can you please take a look...do you have better idea how to do it?
TODO: