-
Notifications
You must be signed in to change notification settings - Fork 53
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
Improve feedback when saving locations, add toast notifications #992
Conversation
…r resending phone verif code.
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! Tested on mobile and AT.
It would be great to move all window.alerts
over to Toast, as this does make the user alerts a little inconsistent, but I think this is a great place to start!!
@@ -138,7 +139,7 @@ export function convertToLegacyLocation(place) { | |||
lon, | |||
// HACK: If a place name and address are provided, put the address in parentheses | |||
// to mimic the existing LocationField behavior for "work" and "home". | |||
// TODO: use addInParentheses from location-field (requires passing an intl contexty). | |||
// TODO: use addInParentheses from location-field (requires passing an intl context). |
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 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.
I think Toast is a cool package, good work! Just one comment
i18n/en-US.yml
Outdated
@@ -52,6 +52,8 @@ actions: | |||
emailVerificationResent: The email verification message has been resent. | |||
genericError: "An error was encountered: {err}" | |||
itineraryExistenceCheckFailed: Error checking whether your selected trip is possible. | |||
mustBeLoggedInToSavePlace: Please log in to save locations. | |||
placeRemembered: "Your {placeType} has been set to: {address}." |
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.
placeType
is either "custom" or "dining" which results in the alert becoming either:
- "Your custom has been set to: Sesame Street."
- "Your dining has been set to: Sesame Street."
Adding something like "place type" would help make this alert more clear
placeRemembered: "Your {placeType} has been set to: {address}." | |
placeRemembered: "Your {placeType} place type has been set to: {address}." |
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.
Good point! I changed the confirmation popup in e1c6453 so it shows the correct place name, and did away with showing the address as there can be other settings for places later.
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 with the change!
Description
This PR improves the user feedback from setting the home or work locations, from the map and the places editor, notably with toast notifications or alerting logged out users they need to log in. Some other confirmation alert messages are also converted into toast notifications.
Under the hood, the added
react-hot-toast
package improves the "politeness" of the confirmation messages shown.PR Checklist