Edit all data doc frontend - #48444
Conversation
davidsbailey
left a comment
davidsbailey
left a comment
There was a problem hiding this comment.
nice work! just one edge case to consider.
| setShowDeleteWarningDialog(false); | ||
| setPendingDeleteDocKey(null); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
This is a bit of an edge case, but I am seeing a problem if I open the edit page in two places, and try to delete the same object twice. I would suggest fixing this while we are thinking about it.
when I try to delete the nonexistent item, I get a 404, which is fine, but the dialog doesn't close, and if you close it manually, the item is still in the list:
What I would suggest is, catch here, and if the status code was 404, just close the dialog and remove the item from the list like you are doing in done above. you could show an error saying the object was already deleted... but... I'm not sure how helpful that is?
There was a problem hiding this comment.
I like that flow, Dave –– don't think an error message is necessary.
To follow-up on Dave's comment, if I block the server from responding, the dialog doesn't close (which is probably good, because it wasn't deleted), but it's unclear what's happening. Can we be okay with that behavior as it's levelbuilder users?
There was a problem hiding this comment.
thank you for catching that, good call!
I wonder if the server isn't responding then the dialog not closing automatically would visually indicate something went wrong moreso than the dialog closing without deleting anything?
There was a problem hiding this comment.
great - glad you are both on board with the proposed 404 behavior.
yes, in case of any error besides 404, it seems fine to leave the dialog open. levelbuilders are usually savvy enough to grab any details from the js error console and send those to us.
|
|
||
| // click delete for data doc with key 'docToDelete' and confirm in dialog | ||
| testDeleteWrapper | ||
| .findAll('.actions-box')[2] |
There was a problem hiding this comment.
Could you base this number on the length of currDocs? I'm worried if anyone changes the length of allDocs, this will fail
There was a problem hiding this comment.
yes great point thank you!
davidsbailey
left a comment
davidsbailey
left a comment
There was a problem hiding this comment.
lgtm after addressing 1 more comment
| ) | ||
| ]); | ||
| setShowDeleteWarningDialog(false); | ||
| setPendingDeleteDocKey(null); |
There was a problem hiding this comment.
please extract duplicate code into a method - I think it meets the criteria that we'll generally want these two codepaths to stay in sync if anything changes.
There was a problem hiding this comment.
oh yes good point, thank you!
| // click delete for data doc with key 'docToDelete' and confirm in dialog | ||
| const docToDeleteActions = testDeleteWrapper | ||
| .findAll('.actions-box') | ||
| .filter(dataDoc => dataDoc.toString().includes(docToDelete.key))[0]; |
megcrenshaw
left a comment
megcrenshaw
left a comment
There was a problem hiding this comment.
Looks great once Dave's comment is addressed! Well done
| isOpen={true} | ||
| handleClose={() => setShowDeleteWarningDialog(false)} | ||
| onCancel={() => setShowDeleteWarningDialog(false)} | ||
| onConfirm={() => deleteDataDoc()} |
There was a problem hiding this comment.
one late tip here - this approach is A+, but also a simple window.confirm is sufficient for confirming levelbuilder actions in the future.
There was a problem hiding this comment.
Gotcha, I'll keep that in mind for the future thank you!
3 participants
This is the frontend part of this Jira ticket (building on the backend PR) for adding an editing interface for the list of all Data Docs, allowing an editor to add a new Data Doc, edit an existing Data Doc, or delete an existing Data Doc.
To see the edit_all page:
Links
Jira ticket: here
Spec: requirement #3 here
Testing story
Localhost testing to run through creating a data doc, editing data docs, and deleting data docs through the edit_all table's functions. In addition, added frontend unit tests.
Follow-up work
Next requirements in the data doc spec.
PR Checklist: