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

fixed some small bugs #840

Merged
merged 5 commits into from
Apr 25, 2024
Merged

fixed some small bugs #840

merged 5 commits into from
Apr 25, 2024

Conversation

grijzea
Copy link
Collaborator

@grijzea grijzea commented Apr 24, 2024

added qualifier to parameter list

added qualifier to parameter list
@grijzea grijzea requested a review from ceesvoesenek April 24, 2024 12:20

allLocations.value = await getLocations()
locations.value = allLocations.value
selectedLocations.value = allLocations.value

parameters.value = await getParameters()
selectedParameters.value = parameters.value
const timeSeriesResults = await getTimeSeriesHeaders()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Top-level await blocks setting up the component until the request finishes, is this what we want?

In any case, since the fetches are independent, they can be executed concurrently with Promise.all or Promise.allSettled.

const timeSeriesResults = await getTimeSeriesHeaders()
allParameters.value = await getParameters()
const parameterQualifiersHeaders: ParameterQualifiersHeader[] = []
timeSeriesResults?.forEach((timeSeriesResult) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be simplified with a .map and a lodash uniqWith maybe

function getParameterName(
parameters: TimeSeriesParameter[],
parameterQualifiersHeader: ParameterQualifiersHeader,
showParameterName: string | undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe showParameterName?: 'name' | 'short name'

@@ -35,6 +35,16 @@ async function downloadFileWithFetch(
}
}

export function filterToParams(filter: Record<string, any>): string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're making a new version of filterToParams, we could also consider using the URL API for query parameters, which might be a bit nicer to work with than strings?

@@ -0,0 +1,6 @@
export interface DataDownloadFilter {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this extend a TimeSeriesFilter (or something), since we are using it on the FEWS PI time series end point?

@@ -0,0 +1,4 @@
export interface ParameterQualifiersHeader {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does @deltares/fews-pi-requests not have an equivalent type? Maybe you can use Pick to extract a subset of this type, to guarantee that the types remain consistent.

@wkramer wkramer merged commit ef2faa2 into main Apr 25, 2024
7 of 8 checks passed
@wkramer wkramer deleted the DWO-698-small-improvements branch April 25, 2024 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants