-
Notifications
You must be signed in to change notification settings - Fork 14k
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
refactor: Bootstrap to AntD - Form - iteration 3 #14502
refactor: Bootstrap to AntD - Form - iteration 3 #14502
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14502 +/- ##
==========================================
+ Coverage 77.06% 77.23% +0.16%
==========================================
Files 958 958
Lines 48242 48292 +50
Branches 6062 5658 -404
==========================================
+ Hits 37179 37299 +120
+ Misses 10862 10791 -71
- Partials 201 202 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
LGTM! Just some inputs/ideas
type="text" | ||
placeholder={t('Label for your query')} | ||
value={label} | ||
onChange={(event: React.ChangeEvent<HTMLInputElement>) => |
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 think we should move away from having to set the values in the component state. The beauty of the Antd forms is that they can handle pretty much everything in the background. We could just set a name in the FormItem and pass the initialValues to the Form as an object. All the rest will be handled internally by Antd. This looks like the perfect case as there is no custom manipulation/side effects of the data. Other forms might gain the same benefit.
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.
@geido I totally agree with you. I noticed that all screens were using forms just to reuse the layout spacing, but none were using validation, state management, etc. This problem is happening everywhere there is a form. The way I'm tackling this is to first get rid of Boostrap and try to avoid adding more refactoring to the components. After that phase, we can revisit all the form components focusing specifically on using the form benefits. This needs a discussion because we need to decide if we're going to use form validation.
f42e7e3
to
d2ba2be
Compare
/testenv up |
@junlincc Ephemeral environment spinning up at http://34.222.126.21:8080. Credentials are |
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
Migrates Form components from Bootstrap to AntD (iteration 3).
See: #10254
@rusackas @junlincc @pkdotson
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
1 - Access the touched components
2 - Check that the layouts and functionality are working
ADDITIONAL INFORMATION