-
Notifications
You must be signed in to change notification settings - Fork 356
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
Angular to react conversion Reconfigure form #8710
Conversation
1ac7f25
to
503ab1f
Compare
503ab1f
to
28ef6b0
Compare
28ef6b0
to
e4c2d4d
Compare
023b7af
to
a1dcb2a
Compare
a1dcb2a
to
9b74178
Compare
All form snapshots were updated because of the |
9b74178
to
829e515
Compare
aad8e40
to
4d595d8
Compare
few minor suggestions
|
For the edit, resize and action headers styling
|
app/javascript/components/reconfigure-vm-form/disk-table/index.jsx
Outdated
Show resolved
Hide resolved
app/javascript/components/reconfigure-vm-form/network-table/index.jsx
Outdated
Show resolved
Hide resolved
4d595d8
to
107abc1
Compare
app/javascript/spec/reconfigure-vm-form/reconfigure-vm-form.spec.js
Outdated
Show resolved
Hide resolved
84656ac
to
0ef41dc
Compare
0ef41dc
to
fb257b0
Compare
app/javascript/components/reconfigure-vm-form/reconfigure-form-fields.js
Outdated
Show resolved
Hide resolved
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.
Looks good to me. Great work @akhilkr128 .
However, would like to have an extra pair of eyes to have a quick test on this..
cc @DavidResende0
50b86ab
to
4e398ff
Compare
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.
From a functional standpoint everything looks good, however I think there's a couple of items we should address before merging:
- The main reconfigure form is missing a reset button
- Both the memory size and type inputs can be moved to the same line as the toggle
- Same thing here
- I think the buttons in the data tables could be smaller. This is more of a project wide enhancement that could be done in a separate pr.
- The way the delete buttons in the table are hidden off screen is going to confuse people. I'm not 100% what the best way to address this is but something should be done before we merge.
f3d1347
to
26e47c3
Compare
cc: @jeffibm |
26e47c3
to
ff3cbd6
Compare
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.
@Fryguy is the failing security check a concern? |
@akhilkr128 , could you rebase this once again. |
ff3cbd6
to
484724d
Compare
Reconfigure form angular to react conversion
484724d
to
9268a85
Compare
Checked commit akhilkr128@9268a85 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint |
This PR doesn't seem to have introduced this change, so I wouldn't worry about it. It would be different if this PR introduced the bad package. |
Before
After
reconfigure-form.mov
Links
Issue - #7603
Steps for Testing/QA
Compute -> Infrastructure -> Virtual Machines -> Configuration -> Reconfigure VM
@miq-bot add_label enhancement