-
Notifications
You must be signed in to change notification settings - Fork 592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move Formik fields and form helpers from @console/dev-console to @console/shared #3516
Changes from all commits
df66dcc
ffa0e75
02260ed
77de593
f775ed4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export { default as ResourceDropdown } from './ResourceDropdown'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,13 @@ | ||
export interface FormFooterProps { | ||
handleSubmit?: () => void; | ||
handleReset: () => void; | ||
handleReset?: () => void; | ||
handleCancel?: () => void; | ||
submitLabel?: string; | ||
resetLabel?: string; | ||
cancelLabel?: string; | ||
isSubmitting: boolean; | ||
errorMessage: string; | ||
successMessage: string; | ||
successMessage?: string; | ||
disableSubmit: boolean; | ||
showAlert?: boolean; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,16 @@ | ||
export { default as InputField } from './InputField'; | ||
export { default as TextAreaField } from './TextAreaField'; | ||
export { default as DropdownField } from './DropdownField'; | ||
export { default as NSDropdownField } from './NSDropdownField'; | ||
export { default as CheckboxField } from './CheckboxField'; | ||
export { default as NumberSpinnerField } from './NumberSpinnerField'; | ||
export { default as EnvironmentField } from './EnvironmentField'; | ||
export { default as DropdownField } from './DropdownField'; | ||
export { default as DroppableFileInputField } from './DroppableFileInputField'; | ||
export { default as EnvironmentField } from './EnvironmentField'; | ||
export { default as InputField } from './InputField'; | ||
export { default as InputSearchField } from './InputSearchField'; | ||
export { default as MultiColumnField } from './multi-column-field/MultiColumnField'; | ||
export { default as ResourceLimitField } from './ResourceLimitField'; | ||
export { default as NSDropdownField } from './NSDropdownField'; | ||
export { default as NumberSpinnerField } from './NumberSpinnerField'; | ||
export { default as RadioButtonField } from './RadioButtonField'; | ||
export { default as ResourceDropdownField } from './ResourceDropdownField'; | ||
export { default as ResourceLimitField } from './ResourceLimitField'; | ||
export { default as SwitchField } from './SwitchField'; | ||
export { default as TextAreaField } from './TextAreaField'; | ||
export * from './field-utils'; | ||
export * from './field-types'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am slightly concerned if we really want to re-export these. It allows to use '@console/shared' for imports but we could hit naming collisions/confusion as the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could definitely create more packages for isolated use, but we need to be very clear with who is dependant on who otherwise we run the risk of circular references. I wonder if we could just export at a deeper level... something like @christianvogt @spadgett thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we circle back to this - you may have a growing issue with rebases as time goes on. Recommend we get this in and circle back to address the export. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, had no idea
SwitchField
was so similar toCheckboxField
... we need to deduplicate this code using some sort of HOC or something. The logic of both of these components is near identical.@christianvogt @rohitkrai03 I recommend we look to clean this up after this PR as to keep everything from crossing wires.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, this didn't come from
dev-console
. I just assumed all fields were in our package. Either way, this should probably be cleaned up, it's near identical code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewballantyne I think we can merge both of these components and control the component it renders internally using a prop. Something like
This is inline with how formik handles rendering form fields with different components. We could make
checkbox
as default variant as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like keeping
SwitchField
as a separate component as it is easier for devs to know it exists. How about this? jtomasek@9f2a976I've used render props pattern and refactored common code into
CheckBoxFieldBase
component. It would be IMHO better to keep that change in a separate PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree, it should be another PR, this one should track the refactor (it's a big enough change to be done on it's own).
FWIW, I lean the render prop way that @jtomasek suggested. I had envisioned more of a function composition where the Component would be passed in and invoked with JSX inside a nested function -- but it seems our hook lint doesn't like that idea. The render prop is a decent alternative imo.