Skip to content
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.

Sign up for GitHub

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

Jump to bottom

PB-901: Modify name of KML file #1071

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

PB-901: Modify name of KML file #1071

wants to merge 8 commits into from

Conversation

sommerfe
Copy link
Contributor

@sommerfe sommerfe commented Sep 18, 2024 •
edited by github-actions bot
Loading

@cypress Cypress
Copy link

cypress bot commented Sep 18, 2024 •
edited
Loading

web-mapviewer    Run #3574

Run Properties:  status check failed Failed #3574  •  git commit 30d5bb9b8e: WIP fix tests
Project web-mapviewer
Run status status check failed Failed #3574
Run duration 04m 13s
Commit git commit 30d5bb9b8e: WIP fix tests
Committer Pascal Barth
View all properties for this run ↗︎
Test results
Tests that failed  Failures 1
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 21
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 210

Tests for review

Failed  tests/cypress/tests-e2e/drawing.cy.js • 1 failed test • e2e/electron/mobile

View Output

Test Artifacts
Drawing module tests > others > can export the drawing/profile in multiple formats Test Replay Screenshots

@sommerfe sommerfe force-pushed the feat-pb-901-kml-change-name-file branch 6 times, most recently from d2006f0 to 912ebb1 Compare September 25, 2024 12:32
@sommerfe sommerfe requested a review from pakb September 30, 2024 07:49
@sommerfe sommerfe marked this pull request as ready for review September 30, 2024 14:19
@sommerfe sommerfe force-pushed the feat-pb-901-kml-change-name-file branch 3 times, most recently from 065f160 to bed4234 Compare October 1, 2024 15:59
@sommerfe
Copy link
Contributor Author

sommerfe commented Oct 1, 2024

Apparently i made a mistake during the rebase so most of my changes are gone, so i have to insert the right code

pakb
pakb reviewed Oct 2, 2024
View reviewed changes
Copy link
Contributor

@pakb pakb left a 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)

src/modules/drawing/components/DrawingToolbox.vue Outdated Show resolved Hide resolved
src/modules/drawing/components/DrawingToolbox.vue Outdated Show resolved Hide resolved
src/modules/drawing/components/DrawingToolbox.vue Outdated Show resolved Hide resolved
src/modules/drawing/components/DrawingToolbox.vue Outdated Show resolved Hide resolved
src/modules/drawing/components/DrawingToolbox.vue Outdated Show resolved Hide resolved
src/store/modules/layers.store.js Outdated Show resolved Hide resolved
@sommerfe sommerfe force-pushed the feat-pb-901-kml-change-name-file branch from f39f35e to 04ce033 Compare October 4, 2024 09:51
@sommerfe sommerfe requested a review from pakb October 4, 2024 10:45
pakb
pakb reviewed Oct 7, 2024
View reviewed changes
Copy link
Contributor

@pakb pakb left a 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"

src/modules/drawing/components/DrawingToolbox.vue Outdated Show resolved Hide resolved
src/store/modules/layers.store.js Outdated Show resolved Hide resolved
tests/cypress/tests-e2e/drawing.cy.js Outdated Show resolved Hide resolved
@sommerfe sommerfe requested a review from pakb October 7, 2024 10:58
pakb
pakb reviewed Oct 8, 2024
View reviewed changes
src/store/modules/layers.store.js Outdated Show resolved Hide resolved
src/modules/drawing/components/DrawingExporter.vue Outdated Show resolved Hide resolved
@sommerfe sommerfe requested a review from pakb October 11, 2024 07:12
@sommerfe sommerfe force-pushed the feat-pb-901-kml-change-name-file branch from 4699f9d to 7100ce1 Compare October 11, 2024 11:35
sommerfe and others added 7 commits October 15, 2024 09:27
@sommerfe @pakb
PB-901: Modify name of KML file
08795bb
@sommerfe @pakb
PB-901: modify name of KML file
20c6b60
@sommerfe @pakb
PB-901: always show kml input
450d451
@sommerfe @pakb
PB-901: fix lint
9314cc8
@sommerfe @pakb
PB-901: rename kml layer from start on
8fe07e8
@sommerfe @pakb
PB-901: keep layer name in component
5c38adf
@pakb
PB-901 : disable drawing name input when no feature are drawn
754c82b 
Adding all relevant i18n keys for this new input.

Showing a tooltip (while disabling the input) telling the user it's not possible to change the drawing name while there is nothing drawn.
This was the easiest way of dealing with this input, as storing the value before a KML was present, or creating an empty one, was really complicated to manage down the line.

So by waiting for a KML (with at least a feature) to be present, we can safely change the KML name without having too much defensive code, or having to store the drawing name in Vuex (or move this part to the DrawingModule, changing a ton of things)
@pakb pakb force-pushed the feat-pb-901-kml-change-name-file branch from 7100ce1 to 754c82b Compare October 15, 2024 07:33
@sommerfe
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pakb pakb Awaiting requested review from pakb

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sommerfe @pakb