Extract model output dropdown to its own component#2839
Conversation
charisk
force-pushed
the
charisk/model-output-dropdown
branch
from
34275be to
4fd8150
Compare
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
A couple of suggestions to make the code simpler, but also LGTM as it is.
| [argumentsList], | ||
| ); | ||
|
|
||
| const enabled = useMemo( |
There was a problem hiding this comment.
Since this is a primitive type and the value is not hard to compute, I don't believe we gain anything by using useMemo. If you'd prefer to not make any changes in this PR I'm happy to open it as a followup.
There was a problem hiding this comment.
I'm following the same pattern as in #2837 so I'd rather keep it consistent. Happy to stick to useMemo or drop it, but would rather do the same across the board.
| modeledMethod, | ||
| onChange, | ||
| }: Props): JSX.Element => { | ||
| const argumentsList = useMemo( |
There was a problem hiding this comment.
argumentList is only used to compute options, so we could merge these together and call geArgumentList on line 27 and it wouldn't change any behaviour. What do you think?
There was a problem hiding this comment.
Ah good point. I could do the same in ModelInputDropdown.
If we keep things as is we would be consistent with ModelInputDropdown and ModelTypeDropdown though.
I don't have strong opinions.
|
I'm going to go ahead and merge this as I have another PR lined up but will apply any feedback in a separate PR. |
2 participants
This will allow us to reuse code for the new method modeling panel.
See similar PR: #2833
Checklist
N/A:
ready-for-doc-reviewlabel there.