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

Updates to Service Explorer, show child services #430

Merged
merged 7 commits into from Jan 16, 2017

Conversation

jeff-phillips-18
Copy link
Member

@jeff-phillips-18 jeff-phillips-18 commented Jan 13, 2017

This PR changes the service explore fields being displayed and adds showing the child services for services that have them.

PT: https://www.pivotaltracker.com/story/show/137174911
Jira: https://patternfly.atlassian.net/browse/CFUX-294

Finishes 137174911

image

image

image

@beanh66 @serenamarie125 @chriskacerguis

@miq-bot add_label euwe/no
@miq-bot add_label ux

@beanh66
Copy link

beanh66 commented Jan 13, 2017

Thanks @jeff-phillips-18 the changes look great!

@serenamarie125
Copy link

@jeff-phillips-18 some of those screens still show the icon near Retires ... but other show Never. Can you confirm that the icon was replaced with Never ?

@jeff-phillips-18
Copy link
Member Author

@serenamarie125 sorry, those were older screenshots. I've updated them.

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.

Overall LGTM! 🤘

Does this work follow design listed below? I was unable to find a link in either the pts or Jira tickets.

https://github.com/ManageIQ/manageiq-design/blob/master/UX/ui-service/MyServices/design.md

<a uib-tooltip="{{item.name}}"
tooltip-placement="bottom"
tooltip-append-to-body="true"
tooltip-append-to-body="true"
Copy link
Member

Choose a reason for hiding this comment

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

Recommend removing duplicate attribute.

<div ng-if=(item.retires_on)>
{{ item.retires_on | date : 'shortDate' }}
</div>
<div translate>Never</div>
Copy link
Member

Choose a reason for hiding this comment

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

Never will always be shown, might want to throw an ng-if on it as well.

<div class="col-lg-2 col-md-2 col-sm-2 hidden-xs">
<div class="list-view-stacked-item"
uib-tooltip="{{item.retires_on ? (item.retires_on | date : 'medium') : ('Never' | translate)}}"
tooltip-append-to-body="true"
Copy link
Member

Choose a reason for hiding this comment

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

These three options:

tooltip-append-to-body="true"
tooltip-popup-delay="1000"
tooltip-placement="bottom"

are repeated many times, recommend using the $uibTooltipProvider to reconfigure the default options in each controller
http://angular-ui.github.io/bootstrap/#/modal

Copy link
Member Author

Choose a reason for hiding this comment

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

@AllenBW Agreed, but I was reluctant as it would effect all screens. Fast tooltips on list views become annoying as you mouse over rows and the tooltips fly by. Is this something we want throughout the application? Only alternative would be to (as you suggest) configure it in each and every controller.

Copy link
Member

Choose a reason for hiding this comment

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

@chriskacerguis @serenamarie125 looking for yo 💭

@jeff-phillips-18 I kinda wanta gingerly say throughout application, one view shouldn't yield a drastically different tooltip experience than the other, so if they are too fast as is, yeah lets slow em all down! and given these options appear to be applicable more often than not, we can put the burden of overriding them on each controller rather than risk the inconsistency of attempting to apply them that way...

ALL of this might be beyond the scope of this pr though (your discretion of course). If so, saul good here 🌮

Copy link
Member Author

Choose a reason for hiding this comment

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

the only parameter that ought to go global would be the popup delay. The tooltip placement is component dependent as is the append-to-body.

I'm thinking the delay setting application wide would be for another PR as individual settings should then be removed from several places.

Copy link
Member

Choose a reason for hiding this comment

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

Ah append-to-body yeah, good call on that, tooltip placement though is set to top by default, more places than not we use it on the bottom though, I would think it to would make sense to make the default whatever we use more often throughout the app and override when necessary.

Will make a pt encompassing this work and start on it today (if you're on board and nothing else flares up)

@jeff-phillips-18
Copy link
Member Author

Updated per comments.

And yes, this is to follow the latest design from https://github.com/ManageIQ/manageiq-design/blob/master/UX/ui-service/MyServices/design.md.

I can add the link to Jira and PT.

@miq-bot
Copy link
Member

miq-bot commented Jan 16, 2017

Checked commits jeff-phillips-18/manageiq-ui-service@abe6775~...fe693bd with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
0 files checked, 0 offenses detected
Everything looks good. 👍

@chriskacerguis
Copy link
Contributor

@AllenBW can you check the update?

@AllenBW
Copy link
Member

AllenBW commented Jan 16, 2017

@chriskacerguis THIS THING IS 🌮

@chriskacerguis
Copy link
Contributor

@AllenBW can you change your approval status?

@chriskacerguis chriskacerguis merged commit 84944ae into ManageIQ:master Jan 16, 2017
@chriskacerguis chriskacerguis modified the milestone: Sprint 53 Ending Jan 31, 2017 Jan 18, 2017
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.

None yet

6 participants