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

Add long_description field into Ansible Playbook catalog item form #4402

Merged
merged 3 commits into from
Aug 6, 2018

Conversation

rvsia
Copy link
Contributor

@rvsia rvsia commented Aug 2, 2018

https://bugzilla.redhat.com/show_bug.cgi?id=1482905

Description of problem:
Unable to add Long Description for Playbook based Catalog Items

Steps to Reproduce:

  1. look at the service catalog, there's a line that says "long description"
  2. go to the catalog item and try to edit it
  3. there is a now way how to edit it

Before/After
screenshot from 2018-08-02 13-09-54

Before
screenshot from 2018-08-02 13-09-51

After
screenshot from 2018-08-02 13-08-38

@miq-bot add_label gaprindashvili/yes, bug

@rvsia rvsia changed the title [WIP] Add long_description field into Ansible Playbook catalog item form Add long_description field into Ansible Playbook catalog item form Aug 2, 2018
@miq-bot miq-bot removed the wip label Aug 2, 2018
@rvsia
Copy link
Contributor Author

rvsia commented Aug 2, 2018

@h-kataria Please, review :)

@rvsia
Copy link
Contributor Author

rvsia commented Aug 2, 2018

@miq-bot remove_label euwe/yes
@miq-bot add_label fine/yes

@miq-bot miq-bot added fine/yes and removed euwe/yes labels Aug 2, 2018
@skateman
Copy link
Member

skateman commented Aug 3, 2018

@rvsia I think you got too many screenshots that look the same 😉. I'm not sure if this is backportable to fine because of Angular. Also you will need to work a little on the commit messages 😉

@miq-bot add_reviewer @epwinchell

@miq-bot miq-bot requested a review from epwinchell August 3, 2018 05:05
@skateman
Copy link
Member

skateman commented Aug 3, 2018

Maybe I'm wrong, but shouldn't be this consistent with the long descripiton for other types? Maybe it's not doable because of Angular, or two tabs would look ugly, so I'm not really sure.

screenshot from 2018-08-03 07-48-17

@skateman
Copy link
Member

skateman commented Aug 3, 2018

Also long description is missing when you just display the item in catalog items:
screenshot from 2018-08-03 08-54-59

@rvsia
Copy link
Contributor Author

rvsia commented Aug 3, 2018

@skateman
The main reason for this solution was that CodeMirror doesn't work well in tabs in Angular form - it renders badly after switching and user has to click on that to show it properly - or there has to be a timeout function which inits the editor and I'm not sure if using timeout functions is a good idea.

And two tabs looks ugly and I don't think it's good design in the old form. You have to click on "Display in Catalog" to show a tab and honestly, I couldn't find "long description" field when I started working on this bug. :D This solution makes more sense - you switch the option and you immediately see what you can change.

I think it would be better to make a PR in the future and redesign both forms (old one and Angular) to look and work the same. There are more differences right now. This is just a BZ fix.

In regarding of showing it in Catalog Items, long description is also missing in other items. And to maintain consistency a "Details" tab should be added to it, because we have "Basic Info" and "Details" tabs in Edit and in Catalog Item there is only a title "Basic Information". It deserves more attention by someone who designs these pages and stuff to make it properly and consinstence, but I don't know how it works here yet. :D

%textarea{"ui-codemirror" => "{mode: 'htmlmixed',
lineNumbers: true,
angular: true,
theme: 'eclipse',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you mention theme explicitly here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a leftover from testing, I'll remove it!

Copy link
Contributor

@epwinchell epwinchell left a comment

Choose a reason for hiding this comment

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

Tested. Looks fine.
43406259-d8022e80-93e9-11e8-8772-24a13ad3213a

@miq-bot
Copy link
Member

miq-bot commented Aug 6, 2018

Checked commits rvsia/manageiq-ui-classic@ad7c96c~...460e6d8 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@mzazrivec mzazrivec self-assigned this Aug 6, 2018
@mzazrivec mzazrivec added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 6, 2018
@mzazrivec mzazrivec merged commit f814beb into ManageIQ:master Aug 6, 2018
@rvsia rvsia deleted the newBZ1482905 branch August 14, 2018 14:16
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