GSLUX-707 activate style edition for drawings #161

mki-c2c · 2024-10-14T14:47:19Z

No user configured icons so far (awaiting myMaps)

JIRA issue

https://jira.camptocamp.com/browse/GSLUX-707

Description

first review to validate the testing approach (for point and polygon):
edit style => check correct URL

Screenshots

(only if the final render is different)

github-actions · 2024-10-14T14:48:23Z

GitHub Pages links: * Luxembourg-geoportail: https://geoportail-luxembourg.github.io/luxembourg-geoportail/GSLUX-707_drawStyle/

tkohr

Thanks for the nice feature @mki-c2c ! Code looks mostly good to me. I left some comments and ideas regarding the testing inline. Don't forget to rebase, as the circle edition has evolved. Shouldn't have an impact here though, if I see right.

Testing it, it works really well. The only thing I noticed that didn't work is the eyedropper stick (not sure if that's what you call it) to choose a color from somewhere else. But it is the same on prod.
Also noticed that you don't have to validate style changes when switching between features. They are preserved, which might be a little strange. But prod also behaves the same here. So averything is isofunctional :-)

tkohr · 2024-10-28T10:55:16Z

cypress/e2e/draw/draw-feat-style.cy.ts

+      cy.window()
+        .its('featureHash')


I think we should be able to wrap this into a cypress function in the commands.ts utilities, where an import of FeatureHash should be possible. Not totally sure, but this would avoid the window workaround.

Then again, there is the question if it makes sense to import logic in the tests that we want to test? An alternative would be to test against the encoded URL values instead of the decodes ones (maybe with the decoded ones in comments). Certainly much less readable, but it would test for potential regressions in FeatureHash.

Or maybe we could manage to test the style properties directly on the openlayers features without using the URL. By getting the map from the window and then the drawLayer => source => feature => style!? Not sure.

I would have loved to import the functions in cypress, but it did not work since the package luxembourg does not exist and must be built first. The window trick seems to be common in tips I found online and we already used it for olMap here: https://github.com/Geoportail-Luxembourg/luxembourg-geoportail/blob/main/cypress/support/commands.ts#L51

I found the e2e test via the URL to be very powerful, the encoding in the URL is a good representation of the app's state

tkohr · 2024-10-28T10:55:30Z

src/components/draw/feature-edit-style.vue

+function onSizeChange(newSize: string | number) {
+  feature.featureStyle.size = parseFloat(newSize as string)
+  feature.changed()
+}
+
+function onAngleChange(newAngle: string | number) {
+  feature.featureStyle.angle = (parseFloat(newAngle as string) * Math.PI) / 180
+  feature.changed()
+}
+
+function onWidthChange(newWidth: string | number) {
+  feature.featureStyle.stroke = parseFloat(newWidth as string)
+  feature.changed()
+}
+
+function onTransparencyChange(newTransparency: string | number) {
+  feature.featureStyle.opacity =
+    (100 - parseFloat(newTransparency as string)) / 100
+  feature.changed()
+}


Shouldn't the input param always be number here, as we use a number input in the html template? This could make the casting and converting unnecessary.

using v-model.number to enforce events as number in the common rangefinder component

tkohr · 2024-10-28T10:55:35Z

src/components/draw/feature-edit-style.vue

            data-cy="featStyleColor"
          />
        </div>
      </div>
    </template>

-    <template v-slot:size>
+    <template v-slot:size="slotProps">


Just for my understanding, where does the slotProps come from?

it' so that I can pass the max parameter for the template configuration
cf <slot name="size" :maxsize="900"></slot>

tkohr · 2024-10-28T10:55:43Z

src/components/draw/feature-item.vue

+// should preform deep copy, nut then the updqte of the OL object on the canvas would not work ??
+const localFeature = props.feature


Is this still an open question?

src/components/draw/feature-item.vue

src/components/draw/feature-sub-content.vue

src/services/state-persistor/utils/FeatureHash.ts

still öissing: line style and point symbol

- different style options - validate => URL - cancel => restore style

…cies between CI and local tests

cypress/e2e/draw/draw-feat-style.cy.ts

AlitaBernachot · 2024-11-05T12:04:21Z

cypress/e2e/draw/draw-feat-style.cy.ts

+        .trigger('input')
+      cy.get('*[data-cy="featStyleLineWidth"]').type('{selectAll}5.5')
+      cy.get('*[data-cy="featStyleTransparency"]').type('{selectAll}22')
+      cy.get('button[data-cy="featureEditValidate"]').click()


why do you wrap all this in a describe statement?

I'll have to check potential reorganisation

AlitaBernachot · 2024-11-05T12:07:31Z

cypress/e2e/draw/draw-feat-style.cy.ts

+    })
+  })
+
+  describe('When checking featureStyle in OL DrawnFeature object', () => {


Actually e2e is meant to mimic user behavior in the browser, so put this statement "When checking featureStyle in OL DrawnFeature object" in a describe for an e2e tests is awkward.

That was the result of some discussion, in theory e2e would be only capable of checking rendered pixels, but this seems a bit too picky. Both checking th URL and checking the OL features' attributes seem a good compromise for meaningful tests

I was talking about the text description, not the test itself.

ah ;-)
that's easier then.

cypress/e2e/draw/draw-feat-style.cy.ts

AlitaBernachot · 2024-11-05T12:11:08Z

package.json

@@ -15,8 +15,8 @@
    "test:unit": "vitest --environment jsdom --root .",
    "test:unit:ci": "vitest run --environment jsdom --coverage",
    "test:e2e": "INSTRUMENT_COVERAGE=true start-server-and-test preview :4173 'cypress open --e2e'",
-    "test:e2e:ci": "INSTRUMENT_COVERAGE=true start-server-and-test 'VITE_USE_PROXYURL=false vite dev --port 4173' :4173 'cypress run --browser chrome --e2e'",
-    "test:e2e:dev": "INSTRUMENT_COVERAGE=true start-server-and-test 'vite dev --port 4173' :4173 'cypress open --browser chrome --e2e'",
+    "test:e2e:ci": "INSTRUMENT_COVERAGE=true start-server-and-test 'VITE_USE_PROXYURL=false vite dev --port 4173' :4173 'cypress run --browser chromium --e2e'",


why this change???

Ok seen together: I didn't know you couldn't run the e2e tests because your are using chromium. But apparently you have found a solution. Thx

yeah, it was not intended to be merged without notice. I wanted the change to go into CI first, with its share of surprises.
Now, cypress is configured to use the first browser among chrome and chromium it will encounter. That should be pretty robust

AlitaBernachot · 2024-11-05T12:12:45Z

src/components/draw/feature-edit-style-symbole.vue

+      <img
+        class="symbol-style"
+        v-if="!!feature.featureStyle.symbolId"
+        :src="`${SYMBOL_ICONS_URL}/symbol/${feature.featureStyle.symbolId}`"


Why "symbol/" is not included in the SYMBOL_ICONS_URL const?

because symbol.url provided by the backend includes symbol/
https://github.com/Geoportail-Luxembourg/luxembourg-geoportail/blob/GSLUX-707_drawStyle/src/components/draw/feature-edit-symbol-list.vue#L55

https://map.geoportail.lu/mymaps/symbols?symboltype=public

AlitaBernachot · 2024-11-05T12:16:10Z

src/components/draw/feature-edit-style.vue

+
+function onSizeChange(newSize: number) {
+  feature.featureStyle.size = newSize
+  feature.changed()


I am not sure the component should be aware of the openlayers feature implementation, it would have been great to refactor and move all this in a service or maybe add new methods to our extended Feature class that already exists.

Agreed, could be cleaner to wrap all this into the DrawnFeature...

AlitaBernachot · 2024-11-12T08:48:37Z

src/components/draw/feature-edit-symbol.vue

  Cross,
  Triangle,
 } as const
 const currentSymbolTab: Ref<tabs> = ref(tabs.configurables)
-const featureColor = 'red' // feature.olFeature.get('color') // TODO: to plug when feature ok
+// const featureColor = feature.featureStyle.color


clean comment

AlitaBernachot · 2024-11-12T08:51:05Z

src/components/draw/feature-edit-symbol.vue

 }

 function onChangeSymbol(id: number) {
-  alert('onChangeSymbol TODO' + id) // TODO:
-  backNavigation()
+  feature.featureStyle.symbolId = `${id}`


Refactor: do this in a class/service, maybe extend the existing one. Same for all the functions above that is changing the feature style.

that's really tricky, if we put a setter on the symbolId, it will not be able to trigger the redraw of its parent, otherwise we have to clone the whole featureStyle.

Actually this listener is the place in the code where access to both symbolId and changed() is possible.

Refactoring is possible, but not sure if these penalties will be worthwhile.

I'll try to write a POC to see if it's not too complex

I converted modifying the featureStyle attributes to cloning the featureStyle object.

I am not quite happy with this change since it slowed down the app quite a lot.

Could you please test if the app is sluggiish for you too ?

The app still works fine for me, I don't see any latency. Thx for the refactor, I was thinking about a function that sets any given property of the style instead of setting the entire style, but you did it this way and it works fine 👌🏽.

AlitaBernachot · 2024-11-12T08:56:03Z

src/components/draw/feature-sub-content.vue

+let prevLabel = feature.label
+let prevDescription = feature.description
+// keep deep copy of previous style to be able to revert style on cancel
+const prevStyle: DrawnFeatureStyle = { ...feature.featureStyle }


What is the logic in keeping the style or the description + label in this specific component? You handle the change in the parent component, it is very hard to find where operations and setters are made.

in case you cancel the editions, label and description are restored. This is not related to the PR, but it was a functional difference with prod

Yes I understand the functionality, my question was: why storing previous values here in this specific feature-sub-content.vue? what is the link with "sub-content"? You could just emit the cancel event, and the parent handles the previous values itself.

Plus I see the label is updated in the treeview but not on the map.

tkohr

Thanks for the adaptions @mki-c2c and for the double-checking in the tests (URL+feature)!

tkohr · 2024-11-12T08:52:56Z

cypress/e2e/background-selection.cy.ts

@@ -22,7 +22,7 @@ describe('Background selector', () => {
      const layers = (<AUTWindowOlMap>window).olMap
        .getLayers()
        .getArray()
-        .filter((l: any) => l.get('cyLayerType') !== 'featureLayer')
+        .filter((l: any) => !/feature(Edit)?Layer/.exec(l.get('cyLayerType')))


A little pity, we can't target the bgLayers directly during filtering, but I guess that's how it is.

tkohr · 2024-11-12T08:53:03Z

cypress/e2e/draw/draw-feat-circle.cy.ts

@@ -35,7 +35,10 @@ describe('Draw "Circle"', () => {
      )
      cy.dragVertexOnMap(200, 200, 300, 300)
      cy.get('*[data-cy="featItemLength"]').should('contain.text', '693.17 km')
-      cy.get('*[data-cy="featItemArea"]').should('contain.text', '38235.40 km²')
+      // test of 2 possible measurements due to precision issues between local tests and CI


I think we can get rid or adapt this comment now :-) (I see you adapted it below)

tkohr · 2024-11-12T08:53:29Z

cypress/e2e/draw/draw-feat-style.cy.ts

+  })
+})
+
+// describe('Test style edition of Point feature', () => {


Did you forget to uncomment or delete this describe ?

tkohr · 2024-11-12T08:54:10Z

src/components/draw/feature-edit-style.vue

+
+function onSizeChange(newSize: number) {
+  feature.featureStyle.size = newSize
+  feature.changed()


Agreed, could be cleaner to wrap all this into the DrawnFeature...

AlitaBernachot · 2024-11-12T08:56:21Z

src/components/draw/feature-sub-content.vue

  currentEditCompKey.value = undefined
 }

 function onClickValidate() {
  const currentComponent =
    editComponents[currentEditCompKey.value as keyof typeof editComponents]

+  prevLabel = feature.label
+  prevDescription = feature.description
+  Object.assign(prevStyle, feature.featureStyle)


Object assign?

well, I'll put prevStyle = {...feature.featureStyle} for homogeneity
Somehow I find
Object.assign(prevStyle, feature.featureStyle)
more concise in this case

ah, I just saw, this writing makes the linter catch a const modification, I see a whole new advantage int the spread syntax.

cool!

AlitaBernachot · 2024-11-12T08:57:33Z

cypress.config.ts

@@ -13,6 +13,14 @@ export default defineConfig({

      // It's IMPORTANT to return the config object
      // with any changed environment variables
+      // console.log(JSON.stringify(config.browsers))


remove unnecessary comments (console.log)

hard to debug

AlitaBernachot · 2024-11-12T08:59:32Z

cypress/e2e/draw/draw-feat-style.cy.ts

+//     cy.visit('/')
+//     describe('Draw "Point"', () => {
+//       cy.get('button[data-cy="drawButton"]').click()
+//       cy.get('button[data-cy="drawPointButton"]').click()


Are we going to keep all these comments?

forgot to delete

AlitaBernachot · 2024-11-12T09:00:05Z

cypress/e2e/layers-selection/layers-selection-catalog.cy.ts

-          .filter((l: any) => l.get('cyLayerType') !== 'featureLayer')
+          .filter(
+            (l: any) =>
+              // regex match


yes we know this is a regex, what is the purpose of this comment?

it helps reading, since nothing tells, this is a regex, I did not know this obscure syntax represents a regex

AlitaBernachot

Please clean your code:

remove unnecessary comments,
use "===" as much as possible (unless it is a choice on purpose),
prefer using {..., something, ... something else } instead of Object.assign() (unless it is on purpose, in some cases I agree Object.assign will be more usefull),
and refacto all functions that sets the feature style.

AlitaBernachot · 2024-11-12T12:16:28Z

Also about UI:

Label for stroke width should be on one line. Border for style buttons should be full black (it looks like only right and bottom borders are black).

AlitaBernachot · 2024-11-12T12:22:19Z

One other UI difference: when modifying a line style in prod, feature is not selected (line width is not thicker) whereas in your PR, the line is thicker and updating the stroke width preview on the map when modifying is not the definite one.

AlitaBernachot · 2024-11-12T12:25:32Z

One other thing: When adding a symbol and setting a size to a point, after zooming out, the symbol keeps the same size. Ok to keep it as is (same behavior in prod).

AlitaBernachot

Ok thx a lot for the rework, I don't see any latency, maybe someone else should test it? ping @tkohr

There are some minor things regarding the UI, it could be great to fix them before merging.

src/services/draw/drawn-feature.ts

tkohr · 2024-11-12T13:17:41Z

Ok thx a lot for the rework, I don't see any latency, maybe someone else should test it? ping @tkohr

No latencies on my side neither :-)

- sync line direction with feature value - refactor line styles into mini-component - sync button state with line style - fix unselect behavior when clicking on map away from features

- move setRadius from composable useEdit to service to prevent creation of multiple listeners (useEdit, must remain singleton) - fix selection, deselection of features between edit modes

mki-c2c · 2024-11-15T15:25:18Z

fixed the UI issues and some more (related to selection deselection)

mki-c2c changed the title ~~activate style edition for drawings~~ GSLUX-707 activate style edition for drawings Oct 15, 2024

mki-c2c requested review from tkohr, AlitaBernachot and tonio and removed request for tkohr October 22, 2024 14:59

tkohr reviewed Oct 28, 2024

View reviewed changes

mki-c2c force-pushed the GSLUX-707_drawStyle branch from 5d2f860 to 398802a Compare November 5, 2024 09:35

mki-c2c added 13 commits November 5, 2024 11:34

activate style edition for drawings

9d0c0d3

still öissing: line style and point symbol

fix type errors

ffda25e

additional style properties (direction, lineStyle)

185b5dd

connect symbol style editor

2bb5b48

select clipart symbols

78e0a77

fix typee

66468e3

write e2e tests

f124d43

fix tests

1bb4384

apply review comments

eccb8e4

implement tests:

9c3add4

- different style options - validate => URL - cancel => restore style

rename feature edit layer for better distinction in tests + cleanup

c7101a8

fix tests

46d066f

fix layer selection

9a92015

mki-c2c force-pushed the GSLUX-707_drawStyle branch from ba81f8f to 9a92015 Compare November 5, 2024 10:34

use set of two potential test results because of resolution descrepan…

4f815ea

…cies between CI and local tests

AlitaBernachot requested changes Nov 5, 2024

View reviewed changes

AlitaBernachot reviewed Nov 5, 2024

View reviewed changes

cypress/e2e/draw/draw-feat-style.cy.ts Outdated Show resolved Hide resolved

AlitaBernachot reviewed Nov 5, 2024

View reviewed changes

mki-c2c requested a review from tkohr November 12, 2024 08:04

AlitaBernachot reviewed Nov 12, 2024

View reviewed changes

tkohr approved these changes Nov 12, 2024

View reviewed changes

AlitaBernachot reviewed Nov 12, 2024

View reviewed changes

AlitaBernachot requested changes Nov 12, 2024

View reviewed changes

mki-c2c added 3 commits November 12, 2024 11:07

review comments

5a98680

refactor but clone featureStyle objects

b9b3a73

fix typing

f8fd266

mki-c2c requested review from AlitaBernachot November 12, 2024 11:44

AlitaBernachot approved these changes Nov 12, 2024

View reviewed changes

tkohr reviewed Nov 12, 2024

View reviewed changes

src/services/draw/drawn-feature.ts Show resolved Hide resolved

mki-c2c added 5 commits November 14, 2024 08:30

deactivate highlighting of feature during style edition

b11af06

improvements of useability:

96a788d

- sync line direction with feature value - refactor line styles into mini-component - sync button state with line style - fix unselect behavior when clicking on map away from features

fix typing

66ef329

some architectural bugfixes:

f7fde7c

- move setRadius from composable useEdit to service to prevent creation of multiple listeners (useEdit, must remain singleton) - fix selection, deselection of features between edit modes

fix test

eae429f

mki-c2c force-pushed the GSLUX-707_drawStyle branch from 225c586 to eae429f Compare November 15, 2024 15:08

mki-c2c merged commit 3345a92 into main Nov 15, 2024
4 checks passed

		// should preform deep copy, nut then the updqte of the OL object on the canvas would not work ??
		const localFeature = props.feature

GSLUX-707 activate style edition for drawings #161

GSLUX-707 activate style edition for drawings #161

Conversation

mki-c2c commented Oct 14, 2024 • edited Loading

JIRA issue

Description

Screenshots

github-actions bot commented Oct 14, 2024

tkohr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mki-c2c Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlitaBernachot Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mki-c2c Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlitaBernachot Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlitaBernachot Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

tkohr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlitaBernachot left a comment • edited Loading

Choose a reason for hiding this comment

AlitaBernachot commented Nov 12, 2024 • edited Loading

AlitaBernachot commented Nov 12, 2024

AlitaBernachot commented Nov 12, 2024

AlitaBernachot left a comment • edited Loading

Choose a reason for hiding this comment

tkohr commented Nov 12, 2024

mki-c2c commented Nov 15, 2024

mki-c2c commented Oct 14, 2024 •

edited

Loading

mki-c2c Oct 28, 2024 •

edited

Loading

AlitaBernachot Nov 5, 2024 •

edited

Loading

mki-c2c Nov 12, 2024 •

edited

Loading

AlitaBernachot Nov 12, 2024 •

edited

Loading

AlitaBernachot Nov 12, 2024 •

edited

Loading

AlitaBernachot left a comment •

edited

Loading

AlitaBernachot commented Nov 12, 2024 •

edited

Loading

AlitaBernachot left a comment •

edited

Loading