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

Move lat, long parsing and validations logic into GeoPoint #2290

Merged

Conversation

ianguerin
Copy link
Collaborator

@ianguerin ianguerin commented Mar 5, 2024

This logic will be removed from Viewfinder in future commit.

#1796

This logic will be removed from Viewfinder in future commit.
@ianguerin ianguerin changed the base branch from main to develop March 5, 2024 21:08
@ianguerin ianguerin requested a review from yvonnesjy March 6, 2024 00:11
@ianguerin ianguerin marked this pull request as ready for review March 6, 2024 00:11
@ianguerin ianguerin self-assigned this Mar 6, 2024
src/js/models/maps/GeoPoint.js Outdated Show resolved Hide resolved
src/js/models/maps/GeoPoint.js Outdated Show resolved Hide resolved
src/js/models/maps/GeoPoint.js Outdated Show resolved Hide resolved
@ianguerin ianguerin changed the base branch from develop to feature-1796-places-autocomplete March 6, 2024 16:06
@robyngit robyngit self-requested a review March 6, 2024 16:08
Copy link
Member

@robyngit robyngit left a comment

Choose a reason for hiding this comment

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

Very nice! I approve this PR once we add some input validation checks, i.e. verifying that variables like value are not only present but also of the expected type (string).

test/js/specs/unit/models/maps/GeoPoint.spec.js Outdated Show resolved Hide resolved
});

describe("Detecting latitude, longitude in string", () => {
it('accepts empty string', () => {
expect(GeoPoint.couldBeLatLong('')).to.be.true;
Copy link
Member

Choose a reason for hiding this comment

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

why could an empty string be a lat, long pair? maybe it's just the name of the function that is confusing me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just mean for this function to disqualify strings from being a lat,long pair and not necessarily that they are a lat,long pair. For example, "13" could be a lat,long pair, because it doesn't have any disqualifying characters in it, though it is not a valid lat,long pair.

src/js/models/maps/GeoPoint.js Show resolved Hide resolved
src/js/models/maps/GeoPoint.js Outdated Show resolved Hide resolved
Copy link
Member

@robyngit robyngit left a comment

Choose a reason for hiding this comment

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

Just one quick change so that parse always returns a response that backbone expects

src/js/models/maps/GeoPoint.js Outdated Show resolved Hide resolved
@robyngit robyngit merged commit 0b1ff8c into NCEAS:feature-1796-places-autocomplete Mar 11, 2024
1 check passed
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.

3 participants