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

BZ#1517101-Adds empty state to service details relationship section #1292

Conversation

AllenBW
Copy link
Member

@AllenBW AllenBW commented Nov 30, 2017

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

When no relationship information was available, we were showing the title with an empty body (made it look like a bug). This fix adds the pf-empty state to the body that clearly indicates no information is available

After (two examples, cuz its nice to see different types)

screen shot 2017-11-30 at 10 13 38 am

screen shot 2017-11-30 at 10 13 38 am

Before (two examples, cuz its nice to see different types)

screen shot 2017-11-30 at 10 14 13 am

screen shot 2017-11-30 at 10 19 32 am

@AllenBW AllenBW added this to the Sprint 75 Ending Dec 11, 2017 milestone Nov 30, 2017
@AllenBW
Copy link
Member Author

AllenBW commented Dec 4, 2017

@serenamarie125 any chance we can get your eyes on this today? 🙇‍♀️ ❤️

@AllenBW AllenBW requested review from chalettu and removed request for ohadlevy December 4, 2017 15:16
Copy link
Contributor

@chalettu chalettu 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

@serenamarie125
Copy link

Hey @AllenBW - in this case, is it that there is no info available or that no relationships exist?

@AllenBW
Copy link
Member Author

AllenBW commented Dec 4, 2017

@serenamarie125 That there is no info available 👍

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.

Please change the text of the empty state pattern to be "No data available", as that matches what we use in other areas.

I think it's an issue that we are showing "No data available" rather than reporting that no relationships exist. But from what you are saying, you don't know the difference between no relationships and you got nothing, so we don't have a choice. @Loicavenel fyi

@himdel
Copy link
Contributor

himdel commented Dec 5, 2017

@serenamarie125 Was that the last issue? Can we ux/approved now that it's fixed? :) Thanks

@@ -32,6 +32,7 @@ function ComponentController ($stateParams, $state, $window, CollectionsApi, Eve
availableTags: [],
credential: {},
listActions: [],
emptyState: {icon: 'pficon pficon-help', title: 'No data available'},
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing i18n? (since it is in the other emptyState)

(and for those getInfo / provInfo messages?)

Copy link
Member Author

Choose a reason for hiding this comment

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

GOOOOOOOD catch ❤️

@AllenBW AllenBW force-pushed the BZ/MASTER/#1517101-service-details-relationship-empty-state branch from 6ab9624 to a7ae903 Compare December 5, 2017 17:43
@AllenBW
Copy link
Member Author

AllenBW commented Dec 5, 2017

@serenamarie125 Wording has been update, following screenshots reflect current wording
screen shot 2017-12-05 at 12 41 24 pm
screen shot 2017-12-05 at 12 42 52 pm

@@ -371,7 +371,9 @@ <h2 translate>Resources</h2>
<div class="panel-body">
<section>
<h2 translate>Relationships</h2>
<div class="container-fluid">
<pf-empty-state ng-if="!vm.service.parent_service || !vm.service.service_template" config="vm.emptyState"></pf-empty-state>
Copy link
Contributor

Choose a reason for hiding this comment

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

&& not ||

(because negating a || b gives !a && !b ;)

Copy link
Contributor

@himdel himdel Dec 5, 2017

Choose a reason for hiding this comment

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

(Unless the goal was really to display "No data available" even if only one of those is missing, but that looks weird, with the list right under it.)

Copy link
Member Author

Choose a reason for hiding this comment

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

good call on the &&!!

updated the pr

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.

LGTM! Thanks @AllenBW

@serenamarie125
Copy link

@miq-bot add_labels ux/review

@serenamarie125
Copy link

@miq-bot add_labels ux/approved

@AllenBW AllenBW force-pushed the BZ/MASTER/#1517101-service-details-relationship-empty-state branch from a7ae903 to ed8ce3c Compare December 5, 2017 18:21
@himdel himdel removed the ux/review label Dec 5, 2017
@AllenBW AllenBW force-pushed the BZ/MASTER/#1517101-service-details-relationship-empty-state branch from ed8ce3c to fea7953 Compare December 5, 2017 18:32
@himdel
Copy link
Contributor

himdel commented Dec 5, 2017

Almost LGTM 👍

Except, oops, one more broken thing..

bad

When you have a service with just vm.service.service_template, that one line is not actually visible in the UI because it's hidden under the window edge..

Looks like somebody forgot to adjust ss-details-wrapper styling when adding a toolbar, or something like that... changing .ss-details-wrapper height from calc(100vh - 98px) to calc(100vh - 149px) makes it look like this..

better

@AllenBW is that something you want to fix in this PR or is this unrelated?

@miq-bot
Copy link
Member

miq-bot commented Dec 5, 2017

Checked commits AllenBW/manageiq-ui-service@a39f37e~...06063c0 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. ⭐

@AllenBW
Copy link
Member Author

AllenBW commented Dec 5, 2017

DANG @himdel fantastic catch <3

screen shot 2017-12-05 at 2 08 02 pm

Updated, looks like we were missing a ss-details-wrapper-with-toolbar class 🤦‍♂️

@himdel
Copy link
Contributor

himdel commented Dec 5, 2017

Perfect @AllenBW thanks! :)

Retested, merging when green..

@himdel himdel merged commit 915ac21 into ManageIQ:master Dec 5, 2017
@AllenBW AllenBW deleted the BZ/MASTER/#1517101-service-details-relationship-empty-state branch December 6, 2017 11:33
simaishi pushed a commit that referenced this pull request Dec 11, 2017
…ils-relationship-empty-state

BZ#1517101-Adds empty state to service details relationship section
(cherry picked from commit 915ac21)

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

Gaprindashvili backport details:

$ git log -1
commit c906bdb0d1d08ff3664fe4dc872ea8456484a9fb
Author: Martin Hradil <himdel@seznam.cz>
Date:   Tue Dec 5 19:14:53 2017 +0000

    Merge pull request #1292 from AllenBW/BZ/MASTER/#1517101-service-details-relationship-empty-state
    
    BZ#1517101-Adds empty state to service details relationship section
    (cherry picked from commit 915ac21ed6b0d1814d8d4c551de3293dac6a945e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1524716

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

7 participants