BGDIINF_SB_2673 show absence of data in the profile#388
Conversation
|
Very nice. |
7a4e5c3 to
66793b0
Compare
|
On mobile by default the tooltip is display below with the description and if we want to see the profile we need to scroll down which is not very user friendly. Especially if the description is empty we only see an empty description and have to scroll down to see the profile. Some possible mitigation
Before it was not optimal but we could see at least part of the profile on an Iphone SE, now we don't see anything and it is then not trivial to see that there is a profile below |
|
I agree with your remark but say this is out of scope of this PR. Here I try to replace the chart generation framework, which is already a big chunk of code to review, I would do what you propose in a second PR/ticket. |
|
Use opentopography freemium API to cover missing data https://opentopography.org/developers |
I prefer to wait for some sort of official solution from our backend, I don't know if users will find this particular solution precise enough when mixed with our data, and that adds a layer of complexity to show the user that this data isn't ours. I think we should think of a more thorough solution and make our backend service world-wide and mercator compatible. |
|
We do already have 2 datasets in the elevation, Swissalti and dhm25 . https://open-elevation.com/ would be my choice |
| }, | ||
| ticks: { | ||
| // processing distance number to be more human-readable | ||
| callback: (val) => round(val * this.factorToUseForDisplayedDistances, 1), |
There was a problem hiding this comment.
Not sure that returning a callback function in computed property is a good idea.
There was a problem hiding this comment.
I don't know either. I don't think we can externalise that (that's a requirement from ChartJS to give a function to process this styling) as we are using some other computed/data props here (this.factorToUseForDisplayedDistance)
| chartJsTooltipConfiguration() { | ||
| return { | ||
| enabled: false, | ||
| external: ({ chart, tooltip }) => { |
There was a problem hiding this comment.
I would avoid returning complex method/callback in computed properties which are cached and reactive.
There was a problem hiding this comment.
then maybe move that to some methods, and have watchers to update the ChartJS instances in case of update... I'll look into it, that's true it's not optimal to have such big objects in the reactive loop
|
The old tooltip was always visible, even at the border: This is not the case anymore of the new tooltip: |
|
The data model source was previously a link that was clickable: This is not anymore the case: Is this a wanted change? |
jedef
left a comment
There was a problem hiding this comment.
There are lots of changes, so I could not review all of it today, so I publish what I already have. Overall I like very much the new style of the graph, that the datapoints are visible and that you can zoom in the graph.
I think that by default the backend always returns 200 points for each request. (but at least every km as the geodesic gemoetry we send to the backend has at least a point every km) I was asking myself if maybe we should adapt the resolution depeding on the length?
|
The problem with the lv95 boundings it that they seem pretty approximative. There is no guarantee that data is available even if within the bonds. ( In the example above the bound is more than 10 km off) Edit: And when the line is long enough the split is done correctly. So maybe this is not a problem with the bound after all and only with how the splitting is done? |
Will detect if line is overlapping LV95 bounds and split it into multiple chunks, so that we can request our backends only with segments that are within LV95 bounds Using some lib that implements the Liang-Barsky algorithm to achieve this, see https://en.wikipedia.org/wiki/Liang%E2%80%93Barsky_algorithm
Add a new "no data" translation key
Adding one class per profile data structure. Implementing a segmenting of the profile before sending backend request, so that we only request data for coordinates that are in LV95 bounds. All out of bound chunks are ignored and filled with a simple Pythagoras distance calculation.
D3 starts to be a bit outdated and hard to handle with the segmentation approach. ChartJS offers a wide variety of customization through plugin, and less direct canvas manipulation (as it functions how we want it to out of the box). Adding two custom ChartJS module to handle the empty/nodata segments of the profile, and one to show the user the data model being used. Using a zoom plugin so that users can now zoom in the profile to better see some feature of it. Rework of the styling of the profile done in the process of switching to ChartJS (e.g. points are now barely visible, but can help to read/understand where the data is)
and to the new ChartJS approach
Instead of re-requesting data from the backend, we can create the CSV data on the fly as we've got everything we need. Removing now unused API function to request profile as a CSV, and fixing the E2E test covering this part of the app. Also fixing the tooltip positioning when hovering the profile chart.
This should clarify why some calculation aren't available, and why part of the UI shouldn't be rendered. Should also behave better in case of malformed/invalid data.
after splitting hasData into two distinct boolean, the tooltip generation went through the refactoring unchanged, this is now fixed.
Manage profile tooltip position so that it doesn't go further than the graph Rework of profile calculation so that each segment now does its own calculation, and the profile make a sum of all wanted values
d39cff9 to
735856d
Compare
Brand new approach to the profile chart generation, we now only request
service-altibackend when we deal with coordinates in LV95's extent, and show the user on the profile chart when there is no data.Also adding the possibility to zoom into the profile with a pinch gesture (mobile) or through a wheel or shift+drag gesture (on desktop)
Test link