Add ability for MethodRow to render multiple modelings of the same method#2910
Add ability for MethodRow to render multiple modelings of the same method#2910robertbrignull merged 10 commits intomainfrom
Conversation
| }); | ||
| } | ||
|
|
||
| function forEachModeledMethod( |
There was a problem hiding this comment.
I'm not totally happy with this but it's the cleanest thing I could come up with right now. The idea is that when there are no modeled methods we still need to render one set of boxes, but passing undefined as the modeled method.
| mode: Mode; | ||
| viewState: ModelEditorViewState; | ||
| revealedMethodSignature: string | null; | ||
| onChange: (modeledMethod: ModeledMethod) => void; |
There was a problem hiding this comment.
I'm not yet sure what the signature for onChange should be. Perhaps it should not be (modeledMethods: ModeledMethod[]) => void so we send over all the modelings for this method whenever anything changes.
I think it'll have to be that, as otherwise is something changes on one modeling, the receiver of the callback wouldn't know which modeling it was. So we have to send all of them at once so it knows if the number of modelings has changed and the exact state of all of them.
I could do that in this PR or the next one. This PR isn't intending to actually make this component work yet and be able to edit multiple modelings. I just want to get the UI looking right and then we can hook up all the implementation later. So long as it still works correctly when the feature flag is disabled that's ok.
There was a problem hiding this comment.
I think sending all of them even if only 1 has changed makes the most sense. My suggestion would be something like onChange: (signature: string, modeledMethods: ModeledMethod[]). I would suggest adding the signature so we don't need to find the signature of the method that is being modeled from the modeledMethods, but can simply use it as-is.
There was a problem hiding this comment.
Making the method signature onChange: (signature: string, modeledMethods: ModeledMethod[]) sounds good to me. I was also considering the same thing. I'll save that change for a separate PR though.
| mode: Mode; | ||
| viewState: ModelEditorViewState; | ||
| revealedMethodSignature: string | null; | ||
| onChange: (modeledMethod: ModeledMethod) => void; |
There was a problem hiding this comment.
I think sending all of them even if only 1 has changed makes the most sense. My suggestion would be something like onChange: (signature: string, modeledMethods: ModeledMethod[]). I would suggest adding the signature so we don't need to find the signature of the method that is being modeled from the modeledMethods, but can simply use it as-is.
| <MultiModelColumn gridColumn={2}> | ||
| {forEachModeledMethod(modeledMethods, (modeledMethod) => ( | ||
| <ModelTypeDropdown | ||
| key={JSON.stringify(modeledMethod)} |
There was a problem hiding this comment.
I think it might actually make sense here to use the index of the item in the array, even though this is not recommended in general. I'm just not sure if the JSON value of the modeled method is a better key since this may lead to duplicates when you create two models with exactly the same values. This may get React even more confused than using an index.
There was a problem hiding this comment.
Thanks for picking up on this. This was a bit of a hack to get the PR finished and I'd forgotten I did this 🙈 . I agree using the JSON value is not ideal.
Maybe using the index is fine in this case because we generally do identify modelings by their index rather than their content.
I've done a quick read of how react keys behave and my understanding is that using the JSON value like this would likely lead to unmounting/mounting a new component for every render. If we use indexes, then it would match up components by their index, and then still see the props have maybe changed and re-render them. But a re-render is much cheaper than creating and mounting a new component.
|
Thanks for the review @koesie10. This should be ready for review again. I've tested manually again and I can't see any changes in behaviour when the feature flag is disabled. |
2 participants
Updates the
MethodRowcomponent but doesn't do all the wiring up to get the data there, so currently this should never display any differently, even if the backend work is complete to be able to load multiple modelings.You can view what it looks like using storybook, and there are a couple of simple tests too.
The appearance should be unaltered when the feature flag is disabled or when there is only one modeling.
This PR doesn't add the icons for adding/removing modelings.
Checklist
ready-for-doc-reviewlabel there.