356 publish dataset #423
356 publish dataset #423
Conversation
…tVersionHasBeenReleased()
|
@ekraffmiller should we move this to Ready for Review? |
| onChange?: (event: React.MouseEvent<HTMLInputElement>) => void | ||
| } | ||
|
|
||
| export function FormRadioGroup({ |
There was a problem hiding this comment.
I am not a fan of cloning nested elements as they may lead to hard debugging in the future.
Nor to limit that a radio group must be forced to be seen in a line with two columns from a specific windows size (sm=3).
But still, I think it serves as a reusable element for most occasions, and in case you want something more customized, you can always compose a radio group in different ways manually without this component.
In my opinion, it is not to my liking, but I think it can be useful for several cases. 👍🏼
There was a problem hiding this comment.
Please describe this new elements in the CHANGELOG under # Non Published Changes
| @@ -0,0 +1,9 @@ | |||
| { | |||
There was a problem hiding this comment.
If this locales are only used in the Dataset page and not in a reusable componente across the application I think we should keep this locales inside the dataset.json file maybe?
| jsVersionUpdateType = JSVersionUpdateType.MINOR | ||
| } else { | ||
| jsVersionUpdateType = JSVersionUpdateType.MAJOR | ||
| } |
There was a problem hiding this comment.
Just a suggestion (I know that this logic is very simple and will be removed in the future.) for safer and cleaner flow control, something like this:
if (versionUpdateType === VersionUpdateType.UPDATE_CURRENT) {
throw new Error('update current version type not supported yet')
}
if (versionUpdateType === VersionUpdateType.MINOR) {
jsVersionUpdateType = JSVersionUpdateType.MINOR
}
if (versionUpdateType === VersionUpdateType.MAJOR) {
jsVersionUpdateType = JSVersionUpdateType.MAJOR
}
| } | ||
| } | ||
| } | ||
| }, [publishCompleted, dataset]) |
There was a problem hiding this comment.
eslint warning in useEffect dependency array.
React Hook useEffect has missing dependencies: 'addDatasetAlert', 'created', 'datasetAlerts', 'navigate', 'publishInProgress', 'removeDatasetAlert', and 'setDatasetAlerts'. Either include them or remove the dependency array
| addDatasetAlert({ messageKey: AlertMessageKey.DATASET_CREATED, variant: 'success' }) | ||
| } | ||
| if (dataset) { | ||
| if (publishInProgress) { |
There was a problem hiding this comment.
Suggestion also, maybe a safer control flow would be:
if (publishInProgress && !publishCompleted) {
// Code here for when a publication is in progress and has not yet been completed.
return statement here to stop the flow
}
if (publishInProgress && publishCompleted) {
// Code here for when a publication is in progress and has been completed.
}
| defaultChecked | ||
| onClick={handleVersionUpdateTypeChange} | ||
| name="update-type" | ||
| label="Minor Version" |
There was a problem hiding this comment.
Labels like "Minor Version", "Major Version" and "Update Current Version" could be in translation file perhaps.
| } | ||
| } | ||
| void pollLocks() | ||
| }, 2000) |
There was a problem hiding this comment.
The interval timing could be stored in this component in a POLL_LOCKS_INTERVAL_TIME exported constant to reuse for testing purposes.
| let intervalId: NodeJS.Timeout | null = null | ||
|
|
||
| if (publishInProgress && dataset) { | ||
| const gotoReleasedPageAfterPublish = async () => { |
There was a problem hiding this comment.
I think the name gotoReleasedPageAfterPublish is outdated for what the function does.
| } | ||
| } | ||
| }, [publishInProgress, dataset, datasetRepository]) | ||
| return publishCompleted |
There was a problem hiding this comment.
An space between the useEffect and the return statement could bring better readability
| @@ -0,0 +1,75 @@ | |||
| /// <reference types="cypress" /> | |||
There was a problem hiding this comment.
Just curious on what type completions does this give us, these are really useful sometimes, I didn't know this one.
What this PR does / why we need it:
Which issue(s) this PR closes:
Closes #356
Special notes for your reviewer:
I wasn't able to add the major and minor version numbers to the popup, because they aren't currently part of the Dataset model. I added an issue for this: #428
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: