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

DDF-04363 Fixed off-by-one error for negative decimal coordinates in tooltip #4364

Merged
merged 2 commits into from
Feb 21, 2019

Conversation

hayleynorton
Copy link
Contributor

@hayleynorton hayleynorton commented Feb 12, 2019

What does this PR do?

Before, the leftPad method in map-info.view.js, which is used in the tooltip's <span> to format the latitude and longitude, was being passed Math.floor(lat) and Math.floor(lon). This caused the off-by-one error because when Math.floor() is evaluated on -179.152798 for example, it returns -180. This PR makes some adjustments to avoid the off-by-one behavior on negative decimal coordinates in both the 2D and 3D maps.

Who is reviewing it?

@Bdthomson
@andrewzimmer
@nsuvarna

Select relevant component teams:

Ask 2 committers to review/merge the PR and tag them here.

@andrewkfiedler
@bdeining
@djblue

How should this be tested?

Follow the steps to reproduce in the issue and verify that you now see the expected behavior

Any background context you want to provide?

What are the relevant tickets?

For GH Issues:
Fixes: #4363

Screenshots

Checklist:

  • Documentation Updated
  • Update / Add Threat Dragon models
  • Update / Add Unit Tests
  • Update / Add Integration Tests

Notes on Review Process

Please see Notes on Review Process for further guidance on requirements for merging and abbreviated reviews.

Review Comment Legend:

  • ✏️ (Pencil) This comment is a nitpick or style suggestion, no action required for approval. This comment should provide a suggestion either as an in line code snippet or a gist.
  • ❓ (Question Mark) This comment is to gain a clearer understanding of design or code choices, clarification is required but action may not be necessary for approval.
  • ❗ (Exclamation Mark) This comment is critical and requires clarification or action before approval.

@coyotesqrl coyotesqrl added this to In progress in GH Issues Pilot Test via automation Feb 13, 2019
Copy link
Contributor

@andrewkfiedler andrewkfiedler left a comment

Choose a reason for hiding this comment

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

It would be worth pulling this function into it's own file and adding a few tests to ensure the behavior doesn't regress in the future.

@@ -14,6 +14,7 @@
**/
/*global require*/
import React from 'react'
import leftPad from '../../react-component/utils/left-pad'
Copy link
Member

Choose a reason for hiding this comment

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

Might consider using String.prototype.padStart here instead.

@bdeining
Copy link
Member

build now

@cxbot
Copy link

cxbot commented Feb 20, 2019

Internal build has been scheduled, your results will be available at build completion.

@cxbot
Copy link

cxbot commented Feb 20, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins.codice.org/job/DDF-Jobs/job/pr/job/Linux/6247/
❌ JOB FAILURE

@hayleynorton
Copy link
Contributor Author

build now

@cxbot
Copy link

cxbot commented Feb 20, 2019

Internal build has been scheduled, your results will be available at build completion.

1 similar comment
@cxbot
Copy link

cxbot commented Feb 21, 2019

Internal build has been scheduled, your results will be available at build completion.

@cxbot
Copy link

cxbot commented Feb 21, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins.codice.org/job/DDF-Jobs/job/pr/job/Linux/6251/
✅ JOB SUCCESS

@andrewzimmer
Copy link
Contributor

Hero Successful 🎉

  • verified stored and displayed lat/long values were the same

@bdeining bdeining merged commit 1606d41 into codice:master Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
GH Issues Pilot Test
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet

9 participants