Implement onClick for the add/remove model buttons in the model editor#2982
Implement onClick for the add/remove model buttons in the model editor#2982robertbrignull merged 4 commits intomainfrom
Conversation
koesie10
left a comment
koesie10
left a comment
There was a problem hiding this comment.
I think the tests will need to be changed since re-rendering won't happen automatically, but the code itself looks good.
| modeledMethods | ||
| .slice(0, modeledMethods.length - 1) | ||
| .map((_, index) => () => { | ||
| const newModeledMethods = [...modeledMethods]; | ||
| newModeledMethods.splice(index, 1); | ||
| onChange(method.signature, newModeledMethods); | ||
| }), |
There was a problem hiding this comment.
Could we make this slightly easier to read by removing the modeledMethods.slice(0, modeledMethods.length - 1) call? It doesn't seem like it would make a huge difference since we wouldn't be using that handler anyway, but I think it would be easier to read.
There was a problem hiding this comment.
Sure, that's fine. Technically we don't need that final handler, but I agree the code is a little cleaner if we just create it anyway. I'm fine with that.
|
|
||
| await userEvent.click(screen.getByLabelText("Add new model")); | ||
|
|
||
| const kindInputs = screen.getAllByRole("combobox", { name: "Model type" }); |
There was a problem hiding this comment.
This doesn't work because the view won't get re-rendered after calling onChange. I think a more useful test here would be to check that onChange gets called with the correct values. Then, you can use the onChange values to re-render and check that there are 2 kind inputs. See for example this test.
There was a problem hiding this comment.
Thanks. I also came to the same conclusion. I've just got all the tests working using the onChange handler.
2 participants
Hooks up the actual
onClickhandler for the add/remove model buttons.Also adds tests. I couldn't think of any more edge cases to test, but if there are some please let me know and I'll add tests for those.
(While testing, I did spot one bug where the method modeling panel didn't update perfectly. I'll open a separate PR for this.)
Checklist
ready-for-doc-reviewlabel there.