Multiple notification channels #899
Conversation
…been set for push notifications
| onRequestPhoneVerificationCode: PhoneCodeRequestHandler | ||
| onSendPhoneVerificationCode: PhoneVerificationSubmitHandler | ||
| phoneFormatOptions: { | ||
| countryCode: string | ||
| } | ||
| } | ||
|
|
||
| const allowedNotificationChannels = ['email', 'sms', 'none'] | ||
| const allowedNotificationChannels = ['email', 'sms', 'push'] |
There was a problem hiding this comment.
Are we to depend on an absence of channels rather than 'none', with this change?
There was a problem hiding this comment.
If the value is 'none' as in the current implementation (or no known value is present), no options will be checked. Now, if no options are checked, should we send '' or 'none' to the server?
There was a problem hiding this comment.
Looking at ibi-group/otp-middleware#185, it seems like either way is possible, however I would side with sending an empty string.
…els are selected.
| <label htmlFor={inputId}> | ||
| <FormattedMessage id="common.notifications.email" /> | ||
| ) : type === 'sms' ? ( | ||
| </label> |
There was a problem hiding this comment.
Might be a bit cleaner to move the label out of these blocks, and conditionally render the i18n id (so you only have this once)
| const inputId = `notification-channel-${type}` | ||
| const inputDescriptionId = `${inputId}-description` | ||
| return ( | ||
| <NotificationOption key={type}> |
There was a problem hiding this comment.
Should these options be in a <ul>?
There was a problem hiding this comment.
The checkboxes are already in a <fieldset>, which is an ARIA group, and it you can simply place input controls in there, see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/fieldset.
There was a problem hiding this comment.
My big question is here is about the push notifications showing up for OTP-RR deployments with no mobile app. I'm worried about the implication of a mobile app existing if a user sees "Register your device using the mobile app" when it doesn't exist for them.
Related to design, I think there is too much vertical space around the phone number options, which makes it easy to miss the push notifications checkbox entirely. I think that's especially the case in your screenshot with more stuff going on. (GitHub won't let me upload a screenshot... will try again later)
| @@ -22,91 +21,97 @@ interface Props extends FormikProps<User> { | |||
| } | |||
| } | |||
|
|
|||
| const allowedNotificationChannels = ['email', 'sms', 'none'] | |||
| const allowedNotificationChannels = ['email', 'sms', 'push'] | |||
There was a problem hiding this comment.
If a particular deployment of OTP-RR doesn't have a mobile app, should the push notification option still appear?
There was a problem hiding this comment.
Very good point. I have added a config parameter in 6954113.
| if ( | ||
| typeof notificationChannel === 'object' && | ||
| typeof notificationChannel.length === 'number' | ||
| ) { |
There was a problem hiding this comment.
There are scenarios where the notificationChannel could be a comma separated string still? Is that in case the user hasn't updated their choice?
There was a problem hiding this comment.
The selected channels are persisted by OTP-middleware as a comma-separated string, see https://github.com/ibi-group/otp-middleware/blob/be241ed8e284281eb6420adb419a7a030c23d297/src/main/java/org/opentripplanner/middleware/models/OtpUser.java#L43, so compatibility with existing accounts is maintained.
There was a problem hiding this comment.
OTP-middleware handles the conversion so it returns an array to the UI.
There was a problem hiding this comment.
Got it, sounds good.
daniel-heppner-ibigroup
assigned binh-dam-ibigroup and daniel-heppner-ibigroup and unassigned daniel-heppner-ibigroup and binh-dam-ibigroup
…h notification settings.
@daniel-heppner-ibigroup I tried adding visual grouping in 344442f. What do you think? (see updated screenshot) |
daniel-heppner-ibigroup
assigned binh-dam-ibigroup and unassigned daniel-heppner-ibigroup
Description
Requires ibi-group/otp-middleware#185.
This PR adds push notifications as an option, describing the number of push devices registered, and allows setting multiple notification channels.
PR Checklist
aria-describedbyused with labels for each notification channel.