Javalab: warn about overwrite to backpack when committing code #42268
Conversation
|
Rerunning Drone, but failing test is unrelated to these code changes. |
apps/src/javalab/CommitDialog.jsx
Outdated
| this.setState({ | ||
| hasBackpackSaveError: false, | ||
| backpackSaveInProgress: false, | ||
| filesToBackpack: [] | ||
| }); | ||
|
|
||
| this.props.backpackApi.getFileList( | ||
| this.handleBackpackLoadError, | ||
| filenames => { | ||
| this.setState({ | ||
| hasBackpackSaveError: false, | ||
| hasBackpackLoadError: false, | ||
| backpackSaveInProgress: false, | ||
| filesToBackpack: [], | ||
| existingBackpackFiles: filenames | ||
| }); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Hmm...I'm wondering if it would be better to keep the previous state update to clear errors and saving in progress state before calling the backpack API? That way, if there's an error loading files, it might be a bit clearer that the file did successfully save, but there was a separate server error reloading the files? Not sure though, because it would also mean back to back state updates.
There was a problem hiding this comment.
Great point, check out the updated version and let me know what you think.
| notes: PropTypes.string, | ||
| onToggleFile: PropTypes.func.isRequired, | ||
| onChangeNotes: PropTypes.func.isRequired | ||
| }; | ||
|
|
||
| export function CommitDialogFile({file, onToggleFile}) { |
There was a problem hiding this comment.
I'm not super up on the react patterns, should this be its own file? Mainly because this file is getting long and it seems like a clean thing to factor out.
There was a problem hiding this comment.
Yeah probably should, we have another sub-component already in this file so I felt like I should either factor both out or neither.
There was a problem hiding this comment.
makes sense, feel free to do as a separate pr if that's easier
There was a problem hiding this comment.
Think I'll do this as a follow-up if that's ok, item here:
|
@molly-moen @sanchitmalhotra126 thanks for the great feedback, think it's a lot cleaner now! Take another look when you have a moment :). |
Provides a warning if you want to commit a file that has the same name as a file in your backpack:
Testing story
Tested manually, and added unit tests. Also tested the error case manually that we show an error message if the files API cannot get the list of files in the backpack currently.