Output settings form to react #5299
Conversation
BundleMonFiles updated (1)
Unchanged files (3)
Total files change +11.07KB +0.09% Final result: ✅ View report in BundleMon website ➡️ |
app/components-react/obs/ObsForm.tsx
Outdated
| options?: Omit<IObsListOption<unknown>, 'description'>[]; | ||
| } | ||
|
|
||
| const ObsInputListCustomInput = forwardRef<{}, IObsInputListCustomInputProps>((p, ref) => { |
There was a problem hiding this comment.
hmm im not sure this component should be named so generically when it has a lot of code dedicated to handling custom resolutions specifically
| /** | ||
| * @remark Note: The following is a copy of the shared component `TextInput` but with the reference forwarded. | ||
| * This is to allow form validation to work correctly. | ||
| */ |
There was a problem hiding this comment.
i wonder if it would make more sense to just forward the refs on these shared components?
There was a problem hiding this comment.
In order to do that, I have to convert the shared component into a forwardRef. Because the TextInput and NumberInput shared components are used in so many other places, there is a risk that this could effect another form. So in order to do this, we'd have to test every form in the app, which we can do but would be a really large task.
| label: $t(tab), | ||
| key: tab, | ||
| })); | ||
| } |
There was a problem hiding this comment.
is there a reason to restructure the props and format them here instead of just doing this formatting before passing in the tabs under the data prop?
There was a problem hiding this comment.
This is how it was done in the vue component.
|
|
||
| const type = settingsFormData[0].parameters[0].currentValue === 'Simple' ? 'collapsible' : 'tabs'; | ||
|
|
||
| return <ObsGenericSettingsForm type={type as IObsFormType} />; |
There was a problem hiding this comment.
looking at how light this component is i wonder if it would make more sense to do more of the output settings specific layout handling here with the tabs and collapsible instead of in ObsGenericSettingsForm, it feels a bit weird to me to put a lot of things specific to one settings page in a generic component
There was a problem hiding this comment.
Using this generic settings form and passing in the backend settings all at once means that the forms will automatically update on the frontend. This will be less frontend maintenance going forward when adding/updating features with settings because all we will need to do is update the realm and the fields will adjust accordingly. Moreover, adding form types to the generic settings form enables the flexibility with form layout without requiring a lot of work on the frontend since all that is needed is to pass in the type. That being said, I will refactor the ObsTabbedFormGroup to be less output settings specific.
No description provided.