Conversation
Passing run #290 ↗︎Details:
Review all test suite changes for PR #593 ↗︎ |
|||||||||||||||
a6d10b9 to
4b6e119
Compare
1009662 to
d06a59e
Compare
087a547 to
44a82c7
Compare
4c0d783 to
44a82c7
Compare
Contributor
Author
|
@pakb this PR is ready, it only has an issue with one drawing test that I haven't found yet the reason of this failing test. Maybe you can already have a look at my code and the test failure to see if you have some good input of the cause of this test failure ? |
pakb
reviewed
Jan 17, 2024
Contributor
pakb
left a comment
There was a problem hiding this comment.
very nicely done! couple questions/small improvements below
16b1ba5 to
1cbce9f
Compare
pakb
approved these changes
Jan 18, 2024
Contributor
pakb
left a comment
There was a problem hiding this comment.
Looking good!
Two points that we can discuss :
- I'm not sure you can do differently, but I don't really like the store as param to some utils file with
updateKmlActiveLayer, if it is possible to move this logic to a store plugin or router plugin, that would be neat. - I find it can be hard to detect that the import UI has been added to the bottom of the screen when you are using a big screen. We could maybe (later or here?) add a little animation (swipe up?) to catch the eye, and make it easier to see that the UI is shown somewhere a bit "unexpected" in regards to the click location
Hovering the mouse on the third party disclaimer icon in a quick way showed a choppy interface as the tooltip quickly appeared and disappeared. To avoid this simply add a small delay.
This is needed in order to speed up boot procedure. For example for the kml layer at startup we need to load the kml and its metadata based on its url, this could be done asynchronously and then update only the kml data of the layer and/or only the kml metadata of the layer. Before we needed to do both together and update them together.
To allow support of local KML file, the file name is used as URL and during the parsing of the URL parameter if the url part of the layer doesn't starts with http then it is ignored and not added to the layer list. I also changed a bit the isLoading logic during the parsing of the url parameter for KML. I did this for 2 reasons: 1. Avoid unnecessary spinner which makes the application feeling to be slow 2. In the case of an external KML layer we cannot load the metadata and it would spin forever Due to his isLoading changes I also updated the load kml plugin to only load kml metadata when the kml is not external and it has missing kml metadata. Simplified KMLLayer class constructor. The fileId can always be parsed from the URL (for non external layer). In order to simplify the constructor which has already quite a lots of parameter remove this one and parse it from the URL whenever possible. Same for the isLoading and isExternal, those two can be guessed from the other parameters.
…ade to vue 3.4 This feature has been adopted in Vue 3.4, therefore it is safe to use it now within vue 3.3. I tried to upgrade to Vue 3.4, but it breaks the drawing module ! Apparently Vue 3.4 has a bug when returning a ref from a composable and watch this ref in a component using the composable. In this case the watch is not fire when the ref changes. This was the case where in `useDrawingModeInteraction.composable.js` the returned ref `lastFinishedFeature` did not fire change event on the watcher in `DawingMarkerInteraction.vue`
The menu section logic was as follow:
1. Open active layers section when a layer was added to an empty list
2. The active layers was open at startup only if the list was not empty
3. The active layers and geo catalogue section were close when opening
any other section
In this logic the active layers was never automatically re-opened unless it went
from empty to non empty.
Now the logic has been changed as follow:
1. By default the active layers is always open (at startup)
2. We have two type of sections:
a. singleMode (formerly nonScrollable): when a section is open, all others are
closed
b. multiMode (formerly scrollable): when a section is open, only singleMode
sections are closed
3. When the drawing and advanced tool section are closed, the active layer section
is automatically open
4. When the import file tool is open, the active layer section is open (closing
the advanced tool section)
This allow for a better user experience and feedback when adding drawing and/or
external layers
In order to debug using the browser debugger tool during cypress tests you need to have the source map and for this you need to provide the vite config to the cypress file pre-processor. Also we need to have the server mode and the cypress mode in sync in order to use our environment variable in tests, otherwise the test will get the prod variable while the server is using the dev variables.
The Vue 3 and Vuex documentation specifies that we should only use javascript plain object as reactive object. Unfortunately we don't do this with our active layers that are javascript class and put into the store. For this use case it is quite convenient and changing this would require a big rework. Therefore we keep the class but with a big warning. The main known issue is that the class getter/setter cannot be reactive which leads to subtle bugs hard to find. Reactivity could also have some issues if the class contains side effect methods (method that set properties). Here I removed all getter/setter and added the warning.
1cbce9f to
0167f1f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To allow support of local KML file, the file name is used as URL and during the
parsing of the URL parameter if the url part of the layer doesn't starts with
http then it is ignored and not added to the layer list.
I also removed the
isLoading = trueduring the parsing of the url parameter forKML. I did this for 2 reasons:
Due to his
isLoadingchanges I also updated the load kml plugin to only loadkml metadata when the kml is not external and it has missing kml metadata.
Test link