Change notification permission handling #15176
Conversation
ClearlyClaire
force-pushed
the
fixes/notification-webpush-banner
branch
from
7734be4 to
05ceb2f
Compare
|
One possible issue with this is that there is no clear indication that notifications won't work when the permission isn't granted (nor explicitly denied) and the banner has been dismissed… |
ClearlyClaire
force-pushed
the
fixes/notification-webpush-banner
branch
2 times, most recently
from
dc54a40 to
99949f4
Compare
|
Added back a message and button for the case where notifications have not been granted (nor explicitly denied).
This appears with the same conditions as the banner, but only in the settings, so it's not nagging, and can still be helpful to diagnose misconfiguration. |
| render () { | ||
| return ( | ||
| <button className='text-btn column-header__setting-btn' tabIndex='0' onClick={this.props.onClick}> | ||
| <Icon id='sliders' /> <FormattedMessage id='notifications.grant_permission' defaultMessage='Grant permission' /> |
There was a problem hiding this comment.
Why is the icon sliders? In the banner, I used the icon to point to the column settings area, but it is not the icon for permissions or desktop notifications.
There was a problem hiding this comment.
This seems to be the most fitting “settings icon” to me, although it's already used for column settings, so I understand it may be confusing. I don't know which icon would be better.
| {alertsEnabled && browserSupport && browserPermission === 'default' && ( | ||
| <div className='column-settings__row column-settings__row--with-margin'> | ||
| <span className='warning-hint'><FormattedMessage id='notifications.permission_required' defaultMessage='Desktop notifications are unavailable because the required permission has not been granted.' /></span> | ||
| <GrantPermissionButton onClick={onRequestNotificationPermission} /> |
There was a problem hiding this comment.
There needs to be margin between the line above and the button.
There was a problem hiding this comment.
I wanted it to be visually grouped with the text.
Would this be better?
There was a problem hiding this comment.
Okay, with the differnet margin to the button below, it's not worse but not better either. If you want it to be visually grouped, why not make the "button" have the same text color and maybe even make it inline part of the text like a link, underlined?
There was a problem hiding this comment.
Something like this?
- allow changing individual alert settings even if permission is not explicitly enabled (asks for permission on toggle) - persist permission request banner dismissal across sessions through settings
ClearlyClaire
force-pushed
the
fixes/notification-webpush-banner
branch
from
99949f4 to
adfd8c5
Compare
|
Changed the permission grant button design according to reviews: |
ClearlyClaire
force-pushed
the
fixes/notification-webpush-banner
branch
from
de78f3b to
570c9b0
Compare
ClearlyClaire
force-pushed
the
fixes/notification-webpush-banner
branch
from
570c9b0 to
276c5c3
Compare
This enables the dismissal of the nagging notification permission banner to persist after a reload or across browsers without breaking the ability to enable notifications later.
Another approach could be to have the alert settings bound to localStorage or to the session, and disable all alerts when dismissing the banner, but that would be a change of behavior and provide an uneasy migration path.