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.

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

[Feature] Improve snapping functionalities #4215

Merged
merged 3 commits into from
Mar 21, 2024

Conversation

mind84
Copy link
Contributor

@mind84 mind84 commented Feb 16, 2024

A bunch of changes on the snapping functionality:

  • make the snap layer transparent, so that it does not overlap with the map layer symbology.
  • disable snap on the hidden layers. If a layer is turned off, then users can refresh the snap to disable snapping on that layer.
  • add a panel to select which layers to snap to.
    Below the snap button I added a panel with the list of snap layers configured for the layer being edited.

Selection_014

Hidden layers are placed at the bottom of this list and users cannot act on them. When a layer is turned off from layertree, then this list is updated accordingly.
Users can turn on/off the snap on visible layers. Then, as for previous point, users can refresh the snap to see the changes.

I'm not sure about this last point, I think it could be useful if there are a lot of snap layers configured. If you think so, I can go ahead with e2e tests, otherwise I'll roll back this last feature and only keep the first two.

You can check this functionality on the test project attached to this PR (form_edition_multilayer_snap.qgs).

Thanks

Funded by Faunalia

@github-actions github-actions bot added this to the 3.8.0 milestone Feb 16, 2024
@Gustry Gustry added sponsored development This development has been funded run end2end If the PR must run end2end tests or not labels Feb 19, 2024
@mind84 mind84 force-pushed the snap_transparency branch from b457ec0 to 4847669 Compare March 10, 2024 15:48
@mind84 mind84 marked this pull request as ready for review March 10, 2024 15:48
Copy link
Collaborator

@rldhont rldhont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, only docstrings.

assets/src/modules/Snapping.js Show resolved Hide resolved
Copy link
Collaborator

@mdouchin mdouchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this feature. I requested to add some text to help the user

assets/src/components/Snapping.js Outdated Show resolved Hide resolved
@rldhont
Copy link
Collaborator

rldhont commented Mar 11, 2024

Cypress failing tests : Feature Toolbar in popup - should display working custom action for the popup feature

@mind84 mind84 force-pushed the snap_transparency branch 2 times, most recently from f8bd1cc to 6a9b7fa Compare March 14, 2024 15:04
@Gustry
Copy link
Member

Gustry commented Mar 15, 2024

I didn't check carefully your PR yet, sorry, I have noticed you have included a Playwright test with some snapping. Simulating mouse drag etc ?

I'm asking for curiosity this because some manual test before each release, one is snapping, because with Cypress, we couldn't integrate it in E2E at that time. CF the current manual test CC @nboisteault

@mind84
Copy link
Contributor Author

mind84 commented Mar 15, 2024

because with Cypress, we couldn't integrate it in E2E at that time

Hi!
I'm curious too because I hit the same problem and I didn't reach that UI interaction level. I'm testing the new panel behavior and the application XHR activities when user disable/enable layer on the layer tree or toggle the snap directly from the new panel (and refresh the snap, of course).

I assume that if the geometry requests to the server are consistent with the user's interactions then the snap works as expected. Maybe not a strong assumption, but not so weak and IHMO acceptable.

So basically yes, it's a manual testing :)

I think I could spend some time to investigate if this can be done with playwright, but not sure about timing.

@Gustry
Copy link
Member

Gustry commented Mar 18, 2024

So basically yes, it's a manual testing :)

Ok, thanks for the explanations.

I'm going to test the PR.

@Gustry
Copy link
Member

Gustry commented Mar 18, 2024

On my screen, new options are quite on the right side, and I have an horizontal scroll bar which is not very good for UX, given the lot of space on the left side :

image

I was also visiting the other snapping project : http://lizmap.local:8130/index.php/view/map?repository=testsrepository&project=form_edition_snap

I guess it's linked to the request from @mdouchin above ?

@mind84
Copy link
Contributor Author

mind84 commented Mar 19, 2024

On my screen, new options are quite on the right side, and I have an horizontal scroll bar which is not very good for UX, given the lot of space on the left side :

Yes, I've placed the new panel in the same control group of the buttons. Initially the center position looks good to me, but in the end I think I will place it left aligned

Thanks

@mind84 mind84 force-pushed the snap_transparency branch from 7b8dac6 to a70069e Compare March 19, 2024 15:13
@mind84 mind84 force-pushed the snap_transparency branch from a70069e to 5b68100 Compare March 19, 2024 15:44
@mind84
Copy link
Contributor Author

mind84 commented Mar 19, 2024

@Gustry

now you should see the list in the right place

Thanks

@Gustry
Copy link
Member

Gustry commented Mar 21, 2024

Thanks, it's a better UX without any horizontal scrollbar like before :

image

Copy link
Member

@Gustry Gustry 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 UX

@Gustry Gustry merged commit 755ee19 into 3liz:master Mar 21, 2024
13 checks passed
@mind84 mind84 deleted the snap_transparency branch March 21, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editing form enhancement run end2end If the PR must run end2end tests or not sponsored development This development has been funded user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants