PB-580: Add warning when drawing could get lost otherwise#1121
PB-580: Add warning when drawing could get lost otherwise#1121
Conversation
web-mapviewer
|
||||||||||||||||||||||||||||
| Project |
web-mapviewer
|
| Branch Review |
develop
|
| Run status |
|
| Run duration | 01m 49s |
| Commit |
|
| Committer | Felix Sommer |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
1
|
Skipped
|
0
|
Passing
|
48
|
| View all changes introduced in this branch ↗︎ | |
ltshb
left a comment
There was a problem hiding this comment.
Implementation looks just, just an issue regarding the store import.
Otherwise I think the warning message should be improved. @hansmannj
@Luke252 a warning about non exported drawing might be a good idea, but in my opinion the warning should say why we should export it or copy the share link. We should also differentiate if the drawing has only be shared via normal link without admin link. So if the user don't click on export and/or copy admin link the warning should say that the drawing cannot be edited anymore once the webpage is reloaded !
pakb
left a comment
There was a problem hiding this comment.
Implementation looks good, but I'm against this message.
It will confuse more than help IMHO.
c1fa52b to
ed2c536
Compare
3656acb to
3cea793
Compare
98d93d1 to
1421bf4
Compare
ltshb
left a comment
There was a problem hiding this comment.
I'm a bit lost here. The PR title says to add a warning when closing the drawing (as the code seems to do) but the warning is not added ? More over based on the previous comments on this PR we should not do it, if it has been decided to do it, it MUST BE documented in the PR. Finally looking at the ticket the changes required seems to have differ but are not 100% clear to me, @hansmannj @Luke252 What has been here decided ? We SHOULD document such decision in ticket. But I'm still quite against such warning, or it should provide a much better info to the user as simply "Attention, your drawing has not been shared.", because such warning is not useful for the purpose of the ticket. I mean maybe the user don't want to share it, but simply recover later on.
ddbaf4a to
068c9c3
Compare
pakb
left a comment
There was a problem hiding this comment.
as discussed yesterday, we will change a bit our approach here, please add me back to review when ready with what is described in the ticket
068c9c3 to
6f372eb
Compare
4719a84 to
bbbcf92
Compare
ltshb
left a comment
There was a problem hiding this comment.
Found some issues while testing:
- No warning set for subsequent drawing, here how to reproduce
a. Create a drawing and save the admin link
b. Close the drawing: no warning as expected because the admin link has been copied
c. Delete the drawing
d. Create a new drawing without copying the new admin link
e. Close the new drawing -> no warning ! In this case the warning should come. - When copying the share link from the drawing, the crosshair param is added to the url (on current dev staging is not the case)
src/modules/i18n/locales/en.json
Outdated
| "draw_type_marker": "Line / surface", | ||
| "drawing_attached": "Drawing added as attachment", | ||
| "drawing_empty_cannot_edit_name": "Please add something to the drawing before editing its name.", | ||
| "drawing_not_shared_admin_warning": "Attention, you have not generated a link to continue to edit your drawing. You won't be able to edit your drawing after closing this window.", |
There was a problem hiding this comment.
@Luke252 @hansmannj For all translation I would say after reloading the website instead of closing this windows (or something like this), because it is more correct, you can close the drawing and re-open it to get the admin link as long as you don't reload the page.
There was a problem hiding this comment.
How about something more explicit, like Attention, if you want to be able to edit your drawing in future, you need an admin link. To get one, please click on share, copy the admin link and save it.
There was a problem hiding this comment.
OK, my suggestion from above does not make much sense, since the admin link is already "thrown at the user" in this case, which makes sense.
So, yes, after reloading the website is fine for me!
Good catch! 👍 |
|
Don't we also want to catch the case, where neither share nor admin link are copied and throw a corresponding warning then? |
Ah, ok, that seem's to be covered already, sorry! |
52d808a to
350d1b1
Compare
ltshb
left a comment
There was a problem hiding this comment.
You still have an issue when opening the share link of the drawing it adds the cross marker
Other issues seems fine now
b7ce9ba to
a000b0b
Compare
9ed796e to
0d62f93
Compare
0d62f93 to
ba153c7
Compare
Test link