feat: events workflow in wizard #969
Conversation
| @@ -65,6 +65,7 @@ describe('generateCode API', () => { | |||
| }, | |||
| }, | |||
| ], | |||
| selectedEvents: [], | |||
There was a problem hiding this comment.
Is there really no other testing that can be performed? It feels odd to see that the only modification to tests needed is adding one line, when enabling some feature.
| setLoading(false); | ||
| }).catch(error => { | ||
| setLoading(false); | ||
| setError('There was an error fetching the data'); | ||
| }); | ||
| }; | ||
|
|
||
| const handleSelectUnselectAll = (action) => { |
There was a problem hiding this comment.
handleSelectUnselectAll is a really confusing name. Do you mean selecting unselect all? Or it works for both? Could you rename this to something else, closer to what it is actually doing?
| @@ -580,20 +513,65 @@ const handleParentChange = (methodName) => { | |||
| }; | |||
|
|
|||
| const handleChildChange = (key) => { | |||
There was a problem hiding this comment.
I've seen these references to things like parents and children. They do not inherently explain what that is. I feel a lot of this code is really living in a context which is really hard to understand if you weren't the one who wrote it. Instead of using terms like child or parent, I would prefer something more declarative. If parent meant say main method and child is arguments, they should just be referenced that way. Let's try to use more direct self explanatory terms.
There was a problem hiding this comment.
Yeah that makes sense. The intent of the naming here is to mock what happens when a user selects a childcheckbox and parent checkbox.
handleParentChange - parent checkbox should select and unselect all child checkboxes
handleChildChange - child checkbox should just check the parent checkbox
Also a bit out of scope for the naming portion but both functions also have to distinguish between which sets of checkboxes they are targeting. Either the events tab or the methods tab beforehand.
The code is setup in a way where if we choose to display more then 1 level it should work with minimal modifications.
low effort but perhaps?
handleChildCheckboxChange
handleParentCheckboxChange
added events into launchpad.