-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add setup screen #470
Add setup screen #470
Conversation
…ea/dataset-configuration into feature/setup-screen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some in-line comments:
i18n/es.po
Outdated
msgid "and {{number}} more." | ||
msgstr "" | ||
|
||
#, fuzzy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove fuzzy comments (check all the files)
// const projectAttributeId = attributes.project.id; | ||
// const projectAttribute = existingAttributes?.find( | ||
// attribute => attribute.attribute.id === projectAttributeId | ||
// ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented code
// attribute => attribute.attribute.id === projectAttributeId | ||
// ); | ||
|
||
const pa = { attribute: { id: attributes.project.id }, value: dataSet.project?.id }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pa -> more declarative name
const createdByAttribute = { attribute: { id: attributes.createdByApp.id }, value: "true" }; | ||
|
||
const attributesToSave = _([pa, createdByAttribute]) | ||
.compactMap(attribute => (attribute.value ? attribute : undefined)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe using filter
?
attr => !attributesToSave.some(save => save.attribute.id === attr.attribute.id) | ||
) || []; | ||
|
||
// Combinar `filteredExisting` con `toSave` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
temporal comment?
}); | ||
} | ||
} | ||
export type ValidateNameOptions = { name: string; dataSetId: Id }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name/dataSetId, both should contain "dataSet" (or none). I'd change all the naming of the use case/types: ValidateNameUseCase -> ValidateDataSetNameUseCase.
return Promise.resolve(getErrors(result.value.error)); | ||
} | ||
} | ||
return Promise.resolve([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if/else if/else
return Promise.resolve([]); | ||
}, | ||
[dataSet, validationInProgressOrError] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too long. The validation stuff can be moved to a custom hook.
expiryDays: 0, | ||
openFuturePeriods: 0, | ||
notifyUser: false, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move to DataSet.initial()
?
return { dataSet, status, updateDataSet }; | ||
} | ||
|
||
export type HttpStatus = "idle" | "loading" | "finished" | "error"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not important, but it looks a bit weird :) Maybe RequestStatus, LoadingStatus or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good, code-wise, just a very minor thing, no re-review needed.
), | ||
}); | ||
}); | ||
return this.getCategoryOptionsByCode(categories.project.code).map(d2Response => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically, we'd make getCategoryOptionsByCode return directly the categoryOptions, no need for the full response. It's a way of narrowing scopes.
📌 References
📝 Implementation
I'm going to create a PR for every step in the wizard in order to avoid a long and complex testing process.
📹 Screenshots/Screen capture
Project-Configuration-app_create_dataset.webm
🔥 Notes to the tester