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

Search component #18

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Search component #18

wants to merge 11 commits into from

Conversation

marySalvi
Copy link
Collaborator

@marySalvi marySalvi commented Jun 20, 2022

Components Added:

  • Search
  • Predicate
  • GeoJsonForm

Expected Functionality:
The Search component pulls in the predicate, geoJsonForm and dateRange components. With Storybook you should be able to interact with all the components however clicking search does (and should) throw an error since it makes an API call.

@marySalvi marySalvi marked this pull request as ready for review June 20, 2022 21:49
});
polyPoints.push(polyPoints[0]);
drawnShape.value.type = 'Polygon';
// This is super duper important!!!!
Copy link
Member

Choose a reason for hiding this comment

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

Can this comment be expanded a little to explain why it's important? It's not immediately clear to me from looking at it why it's super duper important 😄

}
id: number | null;
name: string | null;
}export interface ResultsFilter {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}export interface ResultsFilter {
}
export interface ResultsFilter {

Comment on lines +33 to +41
const isGeoJSON = (inputText: string) => {
try {
JSON.parse(inputText);
geoJsonErrorMessages.value = [];
} catch (e) {
geoJsonErrorMessages.value = ['Not a valid GeoJSON Polygon or MultiPolygon'];
}
return true;
};
Copy link
Member

Choose a reason for hiding this comment

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

This might just be a personal preference on my part, but when I see a function named like isXXX, I expect it to be a pure function that simply checks if something is true. This function does that, but also has the side effect of populating the geoJsonErrorMessages list.

Also, the return type should probably be boolean in all cases -

Suggested change
const isGeoJSON = (inputText: string) => {
try {
JSON.parse(inputText);
geoJsonErrorMessages.value = [];
} catch (e) {
geoJsonErrorMessages.value = ['Not a valid GeoJSON Polygon or MultiPolygon'];
}
return true;
};
const isGeoJSON = (inputText: string) => {
try {
JSON.parse(inputText);
geoJsonErrorMessages.value = [];
} catch (e) {
geoJsonErrorMessages.value = ['Not a valid GeoJSON Polygon or MultiPolygon'];
return false;
}
return true;
};

Comment on lines +54 to +60
const collectionIDs: number[] = [];
// eslint-disable-next-line no-unused-expressions
resultsFilter.value.collections?.forEach((element) => {
if (element.id) {
collectionIDs.push(element.id);
}
});
Copy link
Member

Choose a reason for hiding this comment

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

I believe this can be simplified to

Suggested change
const collectionIDs: number[] = [];
// eslint-disable-next-line no-unused-expressions
resultsFilter.value.collections?.forEach((element) => {
if (element.id) {
collectionIDs.push(element.id);
}
});
const collectionIDs = resultsFilter.value.collections?.filter((element) => element.id).map((element) => element.id);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless element.id has a numeric type, in which case you might need a more specific test (against null or undefined, etc.).

'Upload File',
];
const geoJsonString = ref('');
const geoJsonErrorMessages = ref(['']);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const geoJsonErrorMessages = ref(['']);
const geoJsonErrorMessages = ref([]);

Does other code depend on this starting off with a single empty string as an element?

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