BGDIINF_SB-2472 : better handling of empty profile by pakb · Pull Request #285 · geoadmin/web-mapviewer

pakb · 2022-11-08T10:01:48Z

As we still need to be able to delete a profile, even if empty, I've left the info container and the trash button. Otherwise, if the whole popup was hidden, the user was not able to delete an empty profile anymore.

In E2E test : adding a profile mockup function as it is used in 2 different files, and harmonisation of profile mockup usage. Reactivation of tests that now works as intended.

Test link

ltshb · 2022-11-08T13:16:47Z

src/modules/infobox/components/FeatureProfile.vue

+            // checking if profile has data, if not (outside CH) we hide the profile
+            this.profileHasData = data.length > 0
            this.profileChart.create(data)
            const areaChartPath = this.profileChart.group.select('.profile-area')
            const glass = this.profileChart.glass


If we don't show the create why create the profile chart ?

ltshb · 2022-11-08T13:20:49Z

When drawing a line outside of swiss, then dragging a point inside switzerland we have the following result

jedef

When you draw a line, the whole info container theoretically could the hidden, as the delete button is not in the profile in this case. But I don't now if it's too complex to make a distinction between measure and line here.
Also, when you select a measure drawn outside of switzerland, the whole info box is first shown before the graph part is hidden. But as you have to wait for the response from the backend to know if the profile is displayed or not, this probably can't be fixed easily.

jedef · 2022-11-08T15:06:46Z

tests/e2e-cypress/integration/drawing/profile.cy.js

+                cy.get('[data-cy="profile-popup-area"]').should('not.have.attr', 'd')
+            })
+            it('has a functioning delete button', () => {
+                // Delete is currently not implemented. Will be added in a later commit.


This comment should probably be deleted

jedef · 2022-11-08T15:09:19Z

src/modules/infobox/components/FeatureProfile.vue

            }
            return `${format(Math.round(value), 3)}m`
        },
+        checkIfProfileDataPresent(data) {


This function seems to be called nowhere

As we still need to be able to delete a profile, even if empty, I've left the info container and the trash button. Otherwise, if the whole popup was hidden, the user was not able to delete an empty profile anymore. In E2E test : adding a profile mockup function as it is used in 2 different files, and harmonisation of profile mockup usage. Reactivation of tests that now works as intended.

Removing unnecessary event, fix typo

Overhaul of the d3 chart generation made by the profile popup. - Simplification of many aspects of this generation - Regrouping of (mostly) all d3 related functions in a dedicated file - Removal of artifact code from mf-geoadmin3, or simplification of it - All business calculation related to profile are now made in a API class, as other API files are doing There is still some UI glitches to fix with scrollbar appearing on the side on mobile, or sometimes when the profile is updated as empty.

This way, it's way easier to debug them, and change the app code without having to reload the whole test suite (but it requires a little bit of pre-loading for the first test)

pakb · 2022-11-15T16:17:02Z

Here it comes, big profile rewrite in order to extract most d3 logic in a file, and move all "wiring" with vue in the component.
This should greatly enhance our reading of this part of the app, and our understanding of it.
Enjoy reviewing it 😉

pakb · 2022-11-15T16:18:01Z

There is still some scrollbar issue appearing sometimes, I've decided that I've spent enough time on this for the time being, we can open a ticket to fix this later on if you guys find it is unsettling.

src/api/profile.api.js

ltshb · 2022-11-16T05:11:59Z

src/api/profile.api.js

+    get highestElevation() {
+        if (this.hasData) {
+            const highestPoint = this.points.reduce((previousPoint, currentPoint) => {
+                if (currentPoint.elevation > previousPoint.elevation) {
+                    return currentPoint
+                }
+                return previousPoint
+            })
+            return highestPoint.elevation
+        }
+        return 0
+    }
+
+    /** @returns {Number} The lowest elevation in this profile */
+    get lowestElevation() {
+        if (this.hasData) {
+            const lowestPoint = this.points.reduce((previousPoint, currentPoint) => {
+                if (currentPoint.elevation < previousPoint.elevation) {
+                    return currentPoint
+                }
+                return previousPoint
+            })
+            return lowestPoint.elevation
+        }
+        return 0
+    }


I don't get the difference between highestElevation and maxElevation ? Same for lowestElevation and minElevation

more simplification can be made, I didn't see that I had two time the kind of same extraction of value

src/api/profile.api.js

ltshb · 2022-11-16T05:23:40Z

src/api/profile.api.js

+     * @returns {number} Estimation of hiking time for this profile
+     */
+    get hikingTime() {
+        if (!this.hasData) {


Same as above check that we have at least two points

ltshb · 2022-11-16T05:29:44Z

src/api/profile.api.js

+        return Math.round(
+            this.points
+                .map((currentPoint, index, points) => {
+                    if (index < points.length - 2) {


If we have only two points, then we don't enter the if and the result will be 0 ! I think it should be if (index < points.length - 1) here or if (index <= points.length - 2)

should be fixed if hasData is only true when there is two or more points

ltshb · 2022-11-16T05:31:17Z

src/api/profile.api.js

+    // no setter
+}
+
+export class GeoAdminProfile {


Why calling this GeoAdminProfile ? It seems to me to be a general profile why not AltiProfile ?

I don't like naming things with only one word, like Profile as it is too generic. And I don't really like Alti too, because it doesn't mean anything in English... So I thought of this name as a "good enough" but am open to suggestions

I totally agree with you Profile is a bad name as it is too general. To be honest I find AltiProfile a good choice as Alti is a shorthand for Altitude and it is an altitude profile. But here other ideas

ElevationProfile

GeoProfile

ltshb · 2022-11-16T05:41:52Z

src/modules/infobox/components/FeatureProfile.vue

    },
    computed: {
+        profileHasData() {
+            return this.profileData && this.profileData.length > 0


If I understand correctly, a valid Profile should have at least two point ? Why not either use the GeoadminProfile.hasData method here (would need to modify this method to check for > 1) or even better rename the hasData to isValid. This method could also be used above in the GeoadminProfile class .

we should be using .hasData here, good catch

ltshb · 2022-11-16T06:00:19Z

src/modules/infobox/components/FeatureProfilePlot.vue

+            )
+            lowerIndex = Math.min(lowerIndex, this.profileData.length - 1)
+            // As it is not perfectly regular (maybe as a result of trunking the decimals in the backend?) we still need to correct our first guess with these two while loops. */
+            while (distToSearch < this.profileData.points[lowerIndex].dist) {


I would add guard here to avoid negative index due to bad data

while (lowerIndex > 0 && distToSearch < this.profileData.points[lowerIndex].dist)

That's also some relic of mf-geoadmin3, I don't mind that we enhance this piece of code in the future, but right now it's really low on our priority list. So if you don't mind, we can write a ticket for post MVP task to have a look into optimizing / fixing this part of the code, but for now we can live with it

ltshb

I also had some exceptions while testing

index.33f0af09.js:93 Uncaught (in promise) TypeError: Cannot read properties of null (reading 'clientWidth')
    at p_t (index.33f0af09.js:93:63583)
    at Proxy.updateProfileChart (index.33f0af09.js:93:65865)
p_t @ index.33f0af09.js:93
updateProfileChart @ index.33f0af09.js:93
Promise.then (async)
vN @ index.33f0af09.js:24
onResize @ index.33f0af09.js:93

ltshb · 2022-11-16T06:08:36Z

src/modules/infobox/components/FeatureProfileInformation.vue

+export default {
+    props: {
+        profile: {
+            type: GeoAdminProfile,


We have a Vue warning here because the profile is required but it can be null ! Either remove the required or allow for null value or when instanciating the FeatureProfile create an empty GeoadminProfile instead of null.

good catch, thanks

Remove unused/doubled min/max elevation Profile is considered having data if it contains at least 2 points (1 was enough before) Adding unit test coverage for basic profile calculation (hiking time is excluded for now...) Switching from old style for loops to filter/map/reduce equivalent in profile calculations Adding a npm target to run unit test in watch mode, easier to debug this way

When the total distance of the profile exceeds 10km, the chart now switches back to display km as values on the X axis instead of flooding the user with big meter numbers

ltshb

Good, but I would change the name GeoadminProfile, see comment. Also a possible minor improvement in the unit test.
Overall very good job and clean code 💯

ltshb · 2022-11-16T11:44:33Z

src/api/__tests__/profile.api.spec.js

+    new ProfilePoint(0, [0, 0], 100),
+    new ProfilePoint(50, [0, 50], 200),
+    new ProfilePoint(150, [0, 150], 90),
+    new ProfilePoint(200, [50, 150], 210),


To better test the maxElevation on line 31, I would suggest to change the elevation order here to put the highest in the middle of the array.

good point, I've added the last point so that I could compute a descent/ascent better but didn't think about the maxElevation, I'll switch it with second place.

pakb · 2022-11-16T11:51:46Z

Let's settle on ElevationProfile, I'll also change some component's name to reflect this

Rename also related components and some variable to reflect this change.

So that we thoroughly test that the max elevation is not necessarily the last point.

pakb requested review from jedef and ltshb November 8, 2022 10:01

github-actions bot added the feature label Nov 8, 2022

ltshb reviewed Nov 8, 2022

View reviewed changes

jedef reviewed Nov 8, 2022

View reviewed changes

pakb added 4 commits November 15, 2022 15:56

BGDIINF_SB-2472 : cleanup

c800202

Removing unnecessary event, fix typo

BGDIINF_SB-2472 : run local e2e tests with dev target

8679670

This way, it's way easier to debug them, and change the app code without having to reload the whole test suite (but it requires a little bit of pre-loading for the first test)

pakb force-pushed the feat_BGDIINF_SB-2472_empty_profile_handling branch from 57adb7f to 8679670 Compare November 15, 2022 16:15

pakb requested review from jedef and ltshb November 15, 2022 16:17

ltshb reviewed Nov 16, 2022

View reviewed changes

pakb added 2 commits November 16, 2022 10:52

BGDIINF_SB-2472 : add back switch to km units on chart

76d0b85

When the total distance of the profile exceeds 10km, the chart now switches back to display km as values on the X axis instead of flooding the user with big meter numbers

pakb requested a review from ltshb November 16, 2022 10:05

ltshb approved these changes Nov 16, 2022

View reviewed changes

pakb added 2 commits November 16, 2022 13:00

BGDIINF_SB-2472 : renaming GeoAdminProfile to ElevationProfile

dc0f877

Rename also related components and some variable to reflect this change.

BGDIINF_SB-2472 : switch max elevation point in the middle in unit test

56fcd73

So that we thoroughly test that the max elevation is not necessarily the last point.

pakb merged commit 3539223 into develop Nov 16, 2022

pakb deleted the feat_BGDIINF_SB-2472_empty_profile_handling branch November 16, 2022 12:38

Conversation

pakb commented Nov 8, 2022 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltshb commented Nov 8, 2022

Uh oh!

jedef left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakb commented Nov 15, 2022

Uh oh!

pakb commented Nov 15, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltshb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltshb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakb commented Nov 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pakb commented Nov 8, 2022 •

edited by github-actions bot

Loading