Skip to content
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

Host Edit Form React Conversion #8608

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

setState((state) => ({
...state,
...appendState,
fields,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use fields: [] and avoid the const fields

@MelsHyrule MelsHyrule force-pushed the host-edit-form branch 2 times, most recently from 434aa5b to 791ca7b Compare February 23, 2023 20:14
@MelsHyrule MelsHyrule force-pushed the host-edit-form branch 3 times, most recently from a59d8ff to d6d73c7 Compare March 16, 2023 18:32
@MelsHyrule MelsHyrule force-pushed the host-edit-form branch 2 times, most recently from 0364f7c to fcfe4d8 Compare March 23, 2023 16:21
@MelsHyrule MelsHyrule changed the title [WIP] Host page Host page Mar 23, 2023
@MelsHyrule MelsHyrule changed the title Host page Host Form React Conversion Mar 23, 2023
@MelsHyrule MelsHyrule changed the title Host Form React Conversion Host Edit Form React Conversion Mar 23, 2023
@MelsHyrule
Copy link
Member Author

@miq-bot add-reviewer @DavidResende0
@miq-bot add-reviewer @akhilkr128
@miq-bot add-reviewer @jeffibm
@miq-bot assign @jeffibm

cc @kbrock

@MelsHyrule
Copy link
Member Author

MelsHyrule commented Mar 23, 2023

NOTE: some follow up PRs were created for the schema, they will be needed to be able to properly test against this PR and they should be merged before this PR
ManageIQ/manageiq-providers-vmware#866
ManageIQ/manageiq-providers-ovirt#639
ManageIQ/manageiq-providers-openstack#843

Update: These PRs have all been merged 👌


const asyncValidate = (fields, fieldNames) => new Promise((resolve, reject) => {
const url = initialValues.host_validate_against ? `/api/hosts/${initialValues.host_validate_against}` : `/api/hosts/${ids[0]}`;
console.log(url);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think you forgot this

Copy link
Member

@DavidResende0 DavidResende0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the most part everything looks good, however I did stumble on this minor bug when reseting the form and then canceling, while editing multiple hosts.

Untitled.mov

@MelsHyrule
Copy link
Member Author

Applied the fix @DavidResende0

Copy link
Member

@DavidResende0 DavidResende0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although since this is a bigger pr I wouldn't mind a second set of eyes just in case I missed something.

@jeffibm
Copy link
Member

jeffibm commented Apr 5, 2023

@MelsHyrule , noticed a small change in new ui v/s old ui redirection on cancel event.

Screen.Recording.2023-04-05.at.10.21.56.AM.mov

- if session[:host_items]
- @embedded = true
= render :partial => 'layouts/gtl'
= react('HostEditForm', { :ids => (@host.id ? [@host.id] : session[:host_items]) })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can avoid spaces before and after { }
= react('HostEditForm', {:ids => (@host.id ? [@host.id] : session[:host_items])})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied the change

@jeffibm
Copy link
Member

jeffibm commented Apr 5, 2023

@MelsHyrule , noticed a small change in new ui v/s old ui redirection on cancel event.

just saw David's comment, but could you please cross-check..

Copy link
Member

@jeffibm jeffibm left a 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..

@miq-bot
Copy link
Member

miq-bot commented Apr 10, 2023

Checked commit MelsHyrule@49f1e1b with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
6 files checked, 2 offenses detected

app/views/host/_form.html.haml

  • ⚠️ - Line 14 - Avoid using instance variables in partials views
  • ⚠️ - Line 14 - Layout/TrailingEmptyLines: Final newline missing.

1 similar comment
@miq-bot
Copy link
Member

miq-bot commented Apr 10, 2023

Checked commit MelsHyrule@49f1e1b with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
6 files checked, 2 offenses detected

app/views/host/_form.html.haml

  • ⚠️ - Line 14 - Avoid using instance variables in partials views
  • ⚠️ - Line 14 - Layout/TrailingEmptyLines: Final newline missing.

@MelsHyrule
Copy link
Member Author

@DavidResende0 @jeffibm applied the changes, are we good to merge?

@DavidResende0
Copy link
Member

@DavidResende0 @jeffibm applied the changes, are we good to merge?

I think you meant approved, but yeah I'll merge it now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants