-
Notifications
You must be signed in to change notification settings - Fork 11
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
PB-901: Modify name of KML file #1071
Conversation
web-mapviewer
|
Project |
web-mapviewer
|
Branch Review |
feat-pb-901-kml-change-name-file
|
Run status |
|
Run duration | 04m 32s |
Commit |
|
Committer | Felix Sommer |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
0
|
|
21
|
|
0
|
|
211
|
View all changes introduced in this branch ↗︎ |
d2006f0
to
912ebb1
Compare
065f160
to
bed4234
Compare
Apparently i made a mistake during the rebase so most of my changes are gone, so i have to insert the right 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.
don't keep console.log
when pushing code.
If you feel this log could help us later down the line, you can use the logging utils from src/utils/logging
(there's a debug()
or error()
function that can be imported from this file)
f39f35e
to
04ce033
Compare
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.
From the user's perspective it doesn't really make sense that they can't start by filling the name of what they want to draw and then start drawing.
As you've already used a ref
to store the name of the drawing (of the KML), I'd really want that we can edit this before the KML even exists. In the onSaveName
, we could check that the KML exists before dispatching the change, and watch the activeKmlLayer for initial creation to set the name correctly.
I think this would really be better for user experience, I'm betting on multiple question/helpdesk interactions with question like "Why can't I change the drawing name"
4699f9d
to
7100ce1
Compare
7100ce1
to
754c82b
Compare
When you change the name and then click on "Retour / Terminer Dessin" you are returned to the main menu but there is still the loading indication at the top so there might be an issue |
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.
Last part of the job (and not the easiest) : fix cypress testing 😉
e5463a4
to
e9afe93
Compare
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.
Can you clean up a bit the git log? There's still my WIP
commit message that should not be merged to develop
as they are
You can squash stuff, and remove my commits, that's your work anyway
e9afe93
to
d4b4ac4
Compare
d4b4ac4
to
c7850c9
Compare
Test link