[ui-storagebrowser] adds file preview and save for files #3869
Conversation
| }; | ||
|
|
||
| const handleSave = () => | ||
| save({ |
There was a problem hiding this comment.
Now if we are establishing a pattern it is reasonable be a bit more picky than usual :-)
I thought about this during the other PR review. Doesn't it make more sense to handle the save success and failure here instead of at the top where we get the save function from the hook? That way the logic is grouped together.
save({...}).then(()=>{do stuff}).catch(...)
or if the error handling is done by the hook like in this example
const success = await save({...});
if (success ) {...}
There was a problem hiding this comment.
Based on our conversation earlier, I researched a bit and found that the best flexible/scalable design pattern for this hook would be to have onSuccess and onError hook be placed on both the places, one in the hook initialisation and other while making the save function call.
I have changed the implemented for the hook as well.
Let me know your thoughts on this.
|
|
||
| const StorageFilePage = ({ fileData }: { fileData: PathAndFileData }): JSX.Element => { | ||
| const { t } = i18nReact.useTranslation(); | ||
| const [isEditing, setIsEditing] = React.useState(false); | ||
| const [fileContent, setFileContent] = React.useState(fileData.view?.contents); | ||
| const fileMetaData = useMemo(() => getFileMetaData(t, fileData), [t, fileData]); | ||
|
|
||
| const { loading: isSaving, save } = useSaveData(SAVE_FILE_API_URL, { | ||
| onSuccess: () => { | ||
| setIsEditing(false); |
There was a problem hiding this comment.
I would disable editing as soon as the save button was clicked, not when successful. If save failed I would enable the editing again.
There was a problem hiding this comment.
Make sense. Noted and fixed.
What changes were proposed in this pull request?
How was this patch tested?