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

support for new and old metric plan #486

Merged
merged 7 commits into from
Mar 3, 2024
Merged

Conversation

Algorush
Copy link
Collaborator

@Algorush Algorush commented Feb 28, 2024

Copy link

netlify bot commented Feb 28, 2024

Deploy Preview for 3dstreet-core-builds ready!

Name Link
🔨 Latest commit 906b2e5
🔍 Latest deploy log https://app.netlify.com/sites/3dstreet-core-builds/deploys/65e1761c5e14420008929c36
😎 Deploy Preview https://deploy-preview-486--3dstreet-core-builds.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

delete old test for calcStreetWidth
@kfarr
Copy link
Collaborator

kfarr commented Feb 28, 2024

@Algorush in index.js I would suggest that we rename the variable streetmixSegmentsFeet to streetmixSegmentsMetric

Everything else looks ok from a code review perspective, testing will be next

@Algorush
Copy link
Collaborator Author

Algorush commented Feb 28, 2024

Also might it be better to remove the calcStreetWidth function from streetmix-utils? Because it is simple now. It is used in two places in the code. And instead calculate the width of the street using reduce of segments array in place:

const streetWidth = segments.reduce(
    (streetWidth, segmentData) => streetWidth + segmentData.width, 0);

@kfarr
Copy link
Collaborator

kfarr commented Feb 28, 2024

Also might it be better to remove the calcStreetWidth function from streetmix-utils? Because it is simple now.

sure!

@kfarr kfarr linked an issue Feb 29, 2024 that may be closed by this pull request
@kfarr
Copy link
Collaborator

kfarr commented Mar 1, 2024

This looks good to me. Here was my testing procedure:

I temporarily modified line 92 of index.html https://github.com/3DStreet/3dstreet/blob/main/index.html#L92

old
<a-entity id="default-street" street streetmix-loader set-loader-from-hash></a-entity>

new
<a-entity id="default-street" street streetmix-loader="streetmixAPIURL: http://localhost:7001/examples/streetmix-schema28.json"></a-entity> for testing old schema, and

<a-entity id="default-street" street streetmix-loader="streetmixAPIURL: http://localhost:7001/examples/streetmix-schema30.json"></a-entity> for testing new schema

both seem to work ok with dimensions as expected (per visual inspection)

@kfarr kfarr merged commit 4603a56 into main Mar 3, 2024
6 checks passed
@kfarr kfarr deleted the new-streetmix-metric-plan branch March 3, 2024 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support new streetmix metric plan
2 participants