Skip to content

Conversation

@julien-nc
Copy link
Member

@julien-nc julien-nc commented Dec 7, 2020

Hey there!

Why not fully rewriting Maps UI 😁?

Sorry in advance for the size of this text. It's also a memo.

How to try it

There isn't any DB migration steps involved. It's safe to switch to this branch and then get back to master. Just run make dev to build.

Immediate goals:

  • get rid of JQuery
  • make the interface more stable/solid
  • make it easier to understand/improve the interface
  • simplify tile layers management => implement custom tile layers management in settings

Long term goals:

  • reuse some components in other apps like Photos
  • produce independent components able to access Maps data from outside Nextcloud

Downsides:

While this banch is incompatible with some of the pending PRs, I think it won't be too hard to adapt them and benefit from using Vue.

Todo

  • base map
  • routing
  • search
    • for user data
    • for places
    • for points of interest
    • on left click
  • track me
  • locate control
  • filter slider
  • photos
  • contacts
  • favorites
  • tracks
  • devices
  • my maps

Choices

Map library

Vue2Leaflet has been chosen over mapbox-gl-vue and vuelayers (OpenLayers) for various reasons.

  • It's quite hard to make Mapbox vector tiles work in OpenLayers
  • Avoid loosing Leaflet routing machine which only works with Leaflet and is compatible with many routing engines.
  • Avoid loosing all the other Leaflet plugins we use (easybutton, markercluster, elevation chart, context menu...)

Overall structure

The code structure is quite different than before. It used to be grouped by topic (photos, favorites...) and each controller took care of everything (display stuff on the map, manipulate the data, make network requests etc...). Communication between controllers was mostly done by method calls.
All the data is now managed by the App top component and then passed down to navigation menu and Map stuff. UI Entities are now more independent from each other. Almost all communication is now done via events.

News

Action history

Photo and favorite actions are now stored and it's possible to cancel and redo them. There's a new map control (top right) with cancel and redo buttons.

SearchField

The new SearchField component is used in the search bar and in each routing step. Routing steps used to be "augmented" with JQuery autocompletion. Multiselect (from nextcloud-vue) is now used everywhere. As it was not possible to inject a Vue component in the leaflet-routing-machine control, things have changed there. Integrated step management is now hidden and replaced by a RoutingSteps component which works in parallel of the routing machine. The routing machine itself has been abstracted in a RoutingMachine component.

Moving favorites and photos

Before: Context menu -> click move -> the move cursor changes to a crosshair -> click on target location to move the favorite/photo
After: Enable dragging in navigation menu -> drag'n'drop favorites/photos

Copy link
Collaborator

@tacruc tacruc left a comment

Choose a reason for hiding this comment

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

Nicely Done 👍

Copy link
Contributor

@paulschwoerer paulschwoerer left a comment

Choose a reason for hiding this comment

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

Good job! That looks like it has been a ton of work. I left a few comments, but couldn't dive deeper, yet. Maybe it would make sense to leverage the vuex store for data loading as has been done for the public favorites?

Comment on lines +210 to +278
::v-deep .icon-hand {
opacity: 1;
background-color: var(--color-main-text);
padding: 0 !important;
mask: url('../../img/hand.svg') no-repeat;
mask-size: 16px auto;
mask-position: center;
-webkit-mask: url('../../img/hand.svg') no-repeat;
-webkit-mask-size: 16px auto;
-webkit-mask-position: center;
min-width: 38px !important;
min-height: 36px !important;
}

::v-deep .icon-hand-slash {
opacity: 1;
background-color: var(--color-main-text);
padding: 0 !important;
mask: url('../../img/hand-slash.svg') no-repeat;
mask-size: 16px auto;
mask-position: center;
-webkit-mask: url('../../img/hand-slash.svg') no-repeat;
-webkit-mask-size: 16px auto;
-webkit-mask-position: center;
min-width: 38px !important;
min-height: 36px !important;
}

::v-deep .icon-save {
background-color: var(--color-main-text);
mask: url('../../img/save.svg') no-repeat;
mask-size: 16px auto;
mask-position: center;
-webkit-mask: url('../../img/save.svg') no-repeat;
-webkit-mask-size: 16px auto;
-webkit-mask-position: center;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot of repeated rules here, wouldn't it be nice to only set the values once for all icons?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes indeed. I just struggled a bit with style rules range and stopped touching them when it worked 😁. There sure is a lot of room for factorization.

Comment on lines +87 to +90
map: {
type: Object,
required: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

It's been a while since I've worked with Vue, so please correct me if I'm mistaken here. My understanding is, that Vue will set up watchers for all fields of a prop. A change on one of the fields would then trigger a rerender of the component. Do you think this could get quite costly when passing in the whole Leaflet Map instance?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it re-renders PhotoLayers when a map attribute changes. The mechanism operates at a finer level. It will only re-render what is affected, what uses the changed values.

Comment on lines +20 to +23
export default {
name: 'RoutingMachine',

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you implemented the RoutingMachine as a Vue component, even though it has no associated template?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the reason is it's now easy to plug it in a Leaflet-Vue based map application. Also it can now be treated as a classic component. It has a reactive visible prop. Stuff like that.

@julien-nc
Copy link
Member Author

@paulschwoerer Thanks for the review.

I never got interested in Vuex. If you convince me I'll probably see how it can be done with Vuex.

But then before this the color was random. Now I had 3 categories with the same color.

The color generation is supposed to be the same as before. The 2 first category name letters are used to determine the color. Anyway we need to improve this. What stops us to make a move here is that categories are not really stored in the DB, they are just a string field of the favorite table. It would be way better to have a category table and to use its ID as a foreign key in favorites table. I'm going to first implement everything like it was before. That's one of the first things to do after that.

@paulschwoerer
Copy link
Contributor

Sorry for not getting back to this, I'll try to do a more in-depth review today or tomorrow, if desired.

@paulschwoerer
Copy link
Contributor

So, just had a look at it. A few things:

  • It seems that favourite sharing is broken, at least for me. When adding a favourite location and then clicking on "Create share link", I'm getting a 400 error, which seems to originate from here. Can you think of a reason, why countFavorites() returns 0 here?

  • The Map component is far too large for my personal taste. In think it would be nice to break it up somewhat

  • The icon in the top right corner should maybe be a lighter colour or use a background, as it's hard to see, especially on the satellite view.

  • I also think it would be nice to have cleaner styling on the place popups.

Other than that LGTM!

@julien-nc
Copy link
Member Author

@paulschwoerer Thanks a lot for the review! I agree and acknowledge everything you said 😁. I'll get back to Maps development once I manage to get less busy...

@szaimen
Copy link
Contributor

szaimen commented May 5, 2021

@eneiluj any update on this? :)

@jmporchet
Copy link

Please make this PR a reality 😄
Thanks for your work!!

Julien Veyssier added 14 commits July 6, 2021 12:43
…ain component, all configs ok (webpack, eslint, stylelint etc...)

Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
@tacruc
Copy link
Collaborator

tacruc commented Apr 1, 2022

With all this changes you refer to the commits within thin branch?
Or #701 and #700, too?

@julien-nc
Copy link
Member Author

@tacruc In this branch. I'll check the others later.

@tacruc
Copy link
Collaborator

tacruc commented Apr 1, 2022

So I would suggest merging #700 and #701 as they merge into here and then merging this into master.

tacruc added 12 commits April 1, 2022 18:50
Signed-off-by: Arne Hamann <[email protected]>
Signed-off-by: Arne Hamann <[email protected]>
Signed-off-by: Arne Hamann <[email protected]>
Don't open sidebar, when opening the viewer

Signed-off-by: Arne Hamann <[email protected]>
Clear sidebar if sidebar is closed (onSidebarClose; big X button top right of sidebar)

Signed-off-by: Arne Hamann <[email protected]>
@szaimen
Copy link
Contributor

szaimen commented Apr 14, 2022

🎉🎉🎉🎉🎉

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants