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

Converted Catalog form #5139

Merged
merged 3 commits into from
Feb 25, 2019

Conversation

rvsia
Copy link
Contributor

@rvsia rvsia commented Jan 8, 2019

Description

Converts Services > Catalogs > Catalogs > Configuration > New/Edit to data-driven forms

  • added <hr> component to DDR
  • fixes duallist helper methods (because of changes of format in duallist)

Before
screenshot from 2019-01-23 13-39-08

After
screenshot from 2019-02-18 11-12-45

Checklist

  • Schema
  • Form component
  • Tests
  • Clean up old ruby code (I hope 🙏 )

@miq-bot miq-bot added the wip label Jan 8, 2019
@rvsia rvsia force-pushed the catalog-form-to-react-forms branch 3 times, most recently from 3c31286 to 0ba8b50 Compare January 11, 2019 10:20
@rvsia rvsia changed the title [WIP] Converts Catalogs > Catalog > Add/Edit to react forms [WIP] Refactored Duallist data format and converted Catalog form Jan 11, 2019
@rvsia rvsia force-pushed the catalog-form-to-react-forms branch 4 times, most recently from 38ead6f to c3865b2 Compare January 16, 2019 10:59
@rvsia rvsia changed the title [WIP] Refactored Duallist data format and converted Catalog form [WIP] Converted Catalog form Jan 16, 2019
@rvsia rvsia force-pushed the catalog-form-to-react-forms branch 5 times, most recently from 96c5c9c to 97e0d96 Compare January 23, 2019 08:39
@rvsia
Copy link
Contributor Author

rvsia commented Jan 23, 2019

@miq-bot add_label hammer/no, services, react, refactoring

@rvsia rvsia force-pushed the catalog-form-to-react-forms branch 2 times, most recently from 5f2aa37 to 7ab2c4d Compare January 23, 2019 12:42
@rvsia rvsia changed the title [WIP] Converted Catalog form Converted Catalog form Jan 23, 2019
@miq-bot miq-bot removed the wip label Jan 23, 2019
@rvsia rvsia changed the title Converted Catalog form [WIP] Converted Catalog form Jan 23, 2019
@miq-bot miq-bot added the wip label Jan 23, 2019
@rvsia rvsia force-pushed the catalog-form-to-react-forms branch from 7ab2c4d to 95253b3 Compare January 23, 2019 13:02
@rvsia rvsia changed the title [WIP] Converted Catalog form Converted Catalog form Jan 23, 2019
@miq-bot miq-bot removed the wip label Jan 23, 2019
@rvsia rvsia force-pushed the catalog-form-to-react-forms branch from 95253b3 to c13a3a9 Compare January 23, 2019 13:15
@rvsia
Copy link
Contributor Author

rvsia commented Feb 20, 2019

@ZitaNemeckova fixed! 👍

@ZitaNemeckova
Copy link
Contributor

ZitaNemeckova commented Feb 21, 2019

@rvsia Try to save a Catalog with a name of existing one
image
image
This should be without popup window like on master + comment 👇#5139 (comment)
image

Otherwise it works perfectly in UI 👍

@terezanovotna
Copy link

When we show message "name has been already taken", we need to show what area of UI this message refers to. If we had more fields, the user would have no idea where the error occured.

@Hyperkid123
Copy link
Contributor

When we show message "name has been already taken", we need to show what area of UI this message refers to.

You mean something like focus the field for instance?

@terezanovotna
Copy link

show in the form what exact area needs to be edited or changed. Example is here

@terezanovotna
Copy link

screen shot 2019-02-21 at 15 02 20

@rvsia rvsia force-pushed the catalog-form-to-react-forms branch from 6b58529 to 314c691 Compare February 21, 2019 14:27
@rvsia
Copy link
Contributor Author

rvsia commented Feb 21, 2019

@ZitaNemeckova Great catch! Fixed the error message (@Hyperkid123 please re-re-review catch blocks 🙏 ), and I was trying to get any other error to test it, but this seems as only one which is possible to get from API. Is that right?

@ZitaNemeckova
Copy link
Contributor

@rvsia I think so. I tried every possible combination and this was only API error I got.

@rvsia
Copy link
Contributor Author

rvsia commented Feb 21, 2019

@ZitaNemeckova Ok, great! Thanks!

@Hyperkid123
Copy link
Contributor

@terezanovotna what about async validation? Meaning instead of waiting for submit the form would check the availability of the name (or whatever) when the user is typing?
It might sound strange but from implementation perspective that would be easier.

@terezanovotna
Copy link

that sounds as an even better solution

@rvsia rvsia force-pushed the catalog-form-to-react-forms branch from dd9c62a to 98b9917 Compare February 25, 2019 09:49
@miq-bot
Copy link
Member

miq-bot commented Feb 25, 2019

Checked commits rvsia/manageiq-ui-classic@ac2345d~...98b9917 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@rvsia
Copy link
Contributor Author

rvsia commented Feb 25, 2019

@martinpovolny There is an issue with errors in this form:

When you add a new Catalog with used name it returns 400 error
When you edit some Catalog and change its name to used name it returns 500 error

image

Do you know to whom address this issue?

@Hyperkid123
Copy link
Contributor

Hyperkid123 commented Feb 25, 2019

I think this one is good to go. @martinpovolny can we merge this one? I think all possible bugs have been taken care of.

@terezanovotna Ok we will design some sane way to make this. We will do it in different PR because we want to make a global solution that can be used in future. Right now the form behaves the same way as before and we will add the async validation ASAP.

@martinpovolny martinpovolny merged commit c7641cc into ManageIQ:master Feb 25, 2019
@martinpovolny martinpovolny added this to the Sprint 106 Ending Mar 4, 2019 milestone Feb 25, 2019
@martinpovolny martinpovolny self-assigned this Feb 25, 2019
@himdel
Copy link
Contributor

himdel commented Apr 8, 2019

@rvsia

When you edit some Catalog and change its name to used name it returns 500 error

Can you please open a manageiq-api issue, with the backtrace?

EDIT: ManageIQ/manageiq-api#574

@rvsia
Copy link
Contributor Author

rvsia commented Apr 8, 2019

@himdel ok! 👍

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.

7 participants