Add toasts where sensible #294
Conversation
sorenjohanson
changed the title
| "NODE_COPIED": "Knoten erfolgreich kopiert!", | ||
| "ERRORS": { | ||
| "ROOT_NODE_COPIED": "Es ist nicht möglich, den Hauptknoten zu kopieren!", | ||
| "GENERIC_NODE_COPY_ERROR": "Beim kopieren des Knoten ist ein Fehler aufgetreten." |
There was a problem hiding this comment.
"Kopieren" needs still a capitalized K ;-)
| ); | ||
| this.toastrService.success(successMessage); | ||
| } catch (e) { | ||
| if (e.message == 'The root node can not be copied') { |
There was a problem hiding this comment.
I would suggest always to use strict equality checks with ===
There was a problem hiding this comment.
do we have to check for the message or isn't there a specific class / type field available?
There was a problem hiding this comment.
if not, maybe its a good idea to at least refactor this message as a constant variable. Better in the mmp lib, otherwise here at the top of the file
There was a problem hiding this comment.
Don't think this has a specific type, but strict equality didn't work for some reason (even though e.message should be typeof string, but maybe it's JS doing JS things), was my first preference as well.
Question is if we've got any other errors this could potentially throw - right now I'm just seeing a specific error message when trying to copy the root node, otherwise it doesn't actually error out at all. If we had multiple errors I could see the value in a constant, but not sure if it's worth it for a one-off.
| const showToast = next.queryParamMap.get('showToast'); | ||
| const toastMessage = next.queryParamMap.get('toastMessage'); | ||
|
|
||
| if (showToast === 'true' && toastMessage) { |
There was a problem hiding this comment.
instead of true/false as strings, maybe better to use 1/0?
There was a problem hiding this comment.
wouldnt be just the availability of a toastMessage enough? Or what's the case where I am passing a toast message without the showToast flag?
There was a problem hiding this comment.
Yeah good point about the toastMessage actually :D
| canActivate( | ||
| next: ActivatedRouteSnapshot, | ||
| state: RouterStateSnapshot | ||
| ): Promise<boolean> | boolean { |
There was a problem hiding this comment.
only use one type, e.g. Promise<boolean>. I don't think you need to change anything else, otherwise you can always a return a Promise.resolve(true).
| @@ -35,7 +35,18 @@ export class DialogAboutComponent { | |||
| if (confirm(this.translateService.instant('MODALS.INFO.CONFIRM_DELETE'))) { | |||
| await this.mapSyncService.deleteMap(await this.mapAdminId); | |||
| await this.storageService.remove(this.map.uuid); | |||
| window.location.reload(); | |||
|
|
|||
| const url = new URL(window.location.href); | |||
There was a problem hiding this comment.
can't we use the router.state here as well? I think its a more unified way of accessing the url
There was a problem hiding this comment.
Yeah that was my preferred way of doing it since router.navigate is a much cleaner way of doing things and allows us to separate URL, query params and fragment, but wasn't sure if it was worth it for a one off (esp. considering the component doesn't have the router yet)
|
|
||
| url.hash = fragment; | ||
|
|
||
| window.location.replace(url.toString()); |
There was a problem hiding this comment.
same here, maybe use angular router instead of doing this manually?
|
This is really helpful, I really like these messages where it was unclear before what really happened 😂 👍 |
closes #288