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

Changed where Generic objects list gets rendered on Service details screen #1246

Merged
merged 5 commits into from
Dec 7, 2017
Merged

Changed where Generic objects list gets rendered on Service details screen #1246

merged 5 commits into from
Dec 7, 2017

Conversation

chalettu
Copy link
Contributor

@chalettu chalettu commented Nov 16, 2017

@miq-bot add_label bug
@miq-bot add_label gaprindashvili/yes

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

@chalettu
Copy link
Contributor Author

demo_go
Sample screenshot of fix. This ensures that even if no VM's exist that we show Generic Objects on a service.

@chriskacerguis chriskacerguis self-assigned this Nov 16, 2017
@chalettu
Copy link
Contributor Author

@Loicavenel and @bascar . Could you take a look at this and see if update looks as you are expecting it to.

@chriskacerguis
Copy link
Contributor

@Loicavenel @bascar need approval on this one. It does change the UX a bit, so @serenamarie125 could you please let us know if that is ok?

</div>
</div>

<div ng-if="vm.genericObjects.length > 0">
Copy link
Member

Choose a reason for hiding this comment

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

<div ng-if="vm.genericObjects.length">
will be true when anything is greater than 0

</section>

Copy link
Member

Choose a reason for hiding this comment

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

eeeekkkk if we don't need da space...

@AllenBW
Copy link
Member

AllenBW commented Nov 16, 2017

@chalettu might not be a bad idea to give this area a header kinda similar "Properties" and "Resources" ?
screen shot 2017-11-16 at 1 53 46 pm

Edit: or maybe if this lives inside of resources, something like "compute"?

@chalettu
Copy link
Contributor Author

Ok, updated the screenshot to put this under the Resources section. It will show even in instances when no VM's exist for a service.
updated_go_screen

@AllenBW
Copy link
Member

AllenBW commented Nov 16, 2017

@chalettu oh gosh you're gonna 🔪 me, from the most recent ss, looks like the horizontal bars under above and below resources went away? or maybe my 👁 deceives me...

@chalettu
Copy link
Contributor Author

They are still there. They are so light its hard to see. If you view the video full screen you should see em.

@AllenBW
Copy link
Member

AllenBW commented Nov 16, 2017

Yeah viewed it full screen... seem em around relationships just not resources... 🤔

Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

Code checks out, LG2M 👍 💃

@chalettu
Copy link
Contributor Author

lines
You can see them in this image. They are a little lighter than those other line used by relationships, but that wasn't a change made in this PR. I merely moved a few things around so that GO's show up in the instance no other resources are defined.

Copy link

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

@chalettu, the Resources title is missing in some of the images. Sorry, I need to catchup on these comments. Looks like the mock is updated later.

Is there a before and after screenshot?

@chalettu
Copy link
Contributor Author

@serenamarie125 , please disregard the earlier images. The very last one is the one we implemented and that one does show things being grouped appropriately under the Resources Title.

chriskacerguis
chriskacerguis previously approved these changes Nov 27, 2017
@chriskacerguis
Copy link
Contributor

@chalettu is this ready to go for @serenamarie125 to review?

AllenBW
AllenBW previously approved these changes Nov 29, 2017
Copy link

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

@chalettu the number of objects of that generic type should be in parentheses ...
so in this example km-demo (2)

I know that's part of the design for all resources, although I'm not sure that it's already been implemented for VMs too

@chalettu chalettu dismissed stale reviews from AllenBW and chriskacerguis via 6de3e44 November 30, 2017 16:20
@AllenBW AllenBW assigned AllenBW and unassigned chriskacerguis Dec 4, 2017
@AllenBW
Copy link
Member

AllenBW commented Dec 4, 2017

@chalettu Any update on implementing the requested changes?

@chalettu
Copy link
Contributor Author

chalettu commented Dec 4, 2017

@AllenBW , code is all set for this PR , I was working on getting onto an appliance so I do a screenshot demo of the requested addition. I will double check with Loic to see if we have a appliance to use.

@chalettu
Copy link
Contributor Author

chalettu commented Dec 4, 2017

updated_image

Screenshot showing the updated count header for Generic Object types.

@chalettu chalettu changed the title Changed where Generic objects list gets rendered on Service detials screen Changed where Generic objects list gets rendered on Service details screen Dec 4, 2017
@miq-bot
Copy link
Member

miq-bot commented Dec 4, 2017

Checked commits https://github.com/chalettu/manageiq-ui-service/compare/1cb64e0e8718feaad1a0b6e22655cfd3bb091e4d~...f95d7145e4ab8f331029622b7ed5a543d1a31336 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@chalettu
Copy link
Contributor Author

chalettu commented Dec 4, 2017

@serenamarie125 , can you take a quick look at the updated screen that includes the count of Genric objects? Thanks in advance.

@serenamarie125
Copy link

Location of Generic Objects is 👍 and thanks for adding the count!

Copy link

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

Changes associated with the scope of this PR look good!

Side Note - unsure if generic object icons are being displayed programmatically or not. If not, we should create an additional issue @chalettu

@serenamarie125
Copy link

@miq-bot add_label ux/approved

@serenamarie125
Copy link

@miq-bot remove_label ux/review

Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

LG2M 🌮 💃

@AllenBW AllenBW merged commit 32ca812 into ManageIQ:master Dec 7, 2017
@AllenBW AllenBW deleted the Generic-objects-list branch December 7, 2017 17:48
simaishi pushed a commit that referenced this pull request Dec 11, 2017
Changed where Generic objects list gets rendered on Service details screen
(cherry picked from commit 32ca812)

https://bugzilla.redhat.com/show_bug.cgi?id=1524721
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 995ee638a89426fa08d4d18252813e9d7d0f3742
Author: Allen Wight <[email protected]>
Date:   Thu Dec 7 12:48:41 2017 -0500

    Merge pull request #1246 from chalettu/Generic-objects-list
    
    Changed where Generic objects list gets rendered on Service details screen
    (cherry picked from commit 32ca812a162cf49bbb43c72b0fe931af86bde3cb)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1524721

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.

6 participants