Implement create/clone/edit functionality for collection form#3644
Conversation
|
Skipping CI for Draft Pull Request. |
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
3e3194e to
fa56354
Compare
b9ff0ee to
beeff2d
Compare
968b3bd to
621f668
Compare
06b314c to
6461fb3
Compare
6461fb3 to
d820035
Compare
|
|
||
| request | ||
| .then(() => { | ||
| history.push({ pathname: `${collectionsBasePath}` }); |
There was a problem hiding this comment.
As a follow up, you might compare to policies which goBack after save. For a specific example, there was some consensus among the team that when people have the task to create several collections, it might be convenient and seem intuitive to go back to table page which has Create collection button.
We can brainstorm pro and con on a call, if you like.
There was a problem hiding this comment.
Sequence of titles in browser history is another way to compare pro and con of push versus pop.
There was a problem hiding this comment.
Sure we can discuss this next week. I compared the behavior of policies and integrations when deciding on this vs goBack and went with push since the way the form works seems a little more similar to integrations. In both collections and integrations, the individual view pages are more of a read-only version of the form, while policies has its own dedicated UI for viewing a single policy. I felt like the goBack functionality was a little unclear when going from an editable collection form to a read-only collection form with the changes applied.
| } else if (initialData) { | ||
| if (pageAction.type === 'clone') { | ||
| initialData.name += ' (COPY)'; | ||
| } |
There was a problem hiding this comment.
is that although control flow prevents initialData from referring to defaultCollectionData for clone action, therefore cannot mutate it,
A question for thought (not blocker to merge) is whether there is an alternative control flow which would allow TypeScript to report such a mutation as a problem? Maybe a prior question is does TypeScript report a problem with current code if defaultCollectionData has as const annotation?
There was a problem hiding this comment.
Commenting here because I noticed while reading code, but it is hard to comment on the line above:
{getAxiosErrorMessage(error)}Less important now that you implemented permission checks, but allows any other generic logic.
There was a problem hiding this comment.
I thought about making the defaultCollectionData as const too, but didn't since it would require the Collection type to use readonly for the embeddedCollectionIds and resourceSelector properties. It is something I thought about when working on the dashboards too, but didn't consider it too strongly since it didn't seem to be a pattern that was used extensively elsewhere.
I think I generally just assume that the data structures I use are immutable even if it isn't strictly enforced. (And sometimes take advantage over it not being so strict like in this case.)
I wonder how Formik would handle things if we passed it immutable initial data? A quick look at their source shows they are running state through a reducer so chances are it would work out fine. I can experiment with this in a follow up.
pedrottimark
left a comment
There was a problem hiding this comment.
You are moving forward with a hop, skip, and jump. Or a create, clone, and edit :)
A few comments for your consideration.
Description
Hooks up the "Save" and "Cancel" buttons on the form in order to implement Create/Clone/Edit for collections.
Checklist
If any of these don't apply, please comment below.
Testing Performed
Visit the collection page and click the "Create Collection" button. Fill out the form with relevant details.
Note that there is an additional value for label left blank in the screenshot below:
Attach a couple of collections:
Notice that the "Save" button is disabled since the form is not in a valid state.
Go back to the blank field and enter a value.
Click on the Save button, both the "Save" and "Cancel" buttons will be disabled when the request is in flight. Once the request succeeds, the page will return to the collection list and the new collection will be present.
Clicking on this collection from the list will take you to the read-only view of the collection, with the details matching the values that were entered during creation.
Scroll back to the top of the page and select "Edit collection" from the Action dropdown. The form fields will be enabled and become editable.
Make some changes to the form, observing that invalid values cause the "Save" button to be disabled like in the "create" mode above.
Fill in the missing values and submit the form.
After being redirected to the collections table, select this collection again and verify that the field values have been updated correctly and that a new collection has not been created.
From the collections table, open the table menu for this collection and select "Clone collection".
The collection form will be presented with the text " (COPY)" appended to the collection name.
Make some changes to the rules and save the collection.
Notice on the collection table page that a new collection has been created. Visit the collection page for each of these collections and verify that they contain different rule sets.
For any collection, view the edit or clone page, and then visit the "Create collection" page. Scrolling to the bottom of the form on any of these pages and clicking the "Cancel" button will redirect you to the collections table.
This test requires manual editing of the mock server.
For the create, edit, and clone modes, click Save and view the result when the server returns an error. A toast should be displayed and the user will remain on the form page: