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

mgr/dashboard: Remove top-right actions text and add "About" page #22762

Merged
merged 3 commits into from
Jul 5, 2018

Conversation

ricardoasmarques
Copy link
Contributor

This PR removes actions text from the top-right toolbar:

screenshot from 2018-06-28 16-07-46

and adds "About" page:

screenshot from 2018-06-28 15-20-06

Fixes: https://tracker.ceph.com/issues/24624

@callithea
Copy link
Member

jenkins retest this please

Copy link
Contributor

@a2batic a2batic left a comment

Choose a reason for hiding this comment

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

@ricardoasmarques according to proposal, the about modal should be patternfly( http://www.patternfly.org/pattern-library/communication/about-modal/ ) based.

@ricardoasmarques
Copy link
Contributor Author

@a2batic by “patternfly based” you mean that I should use the same HTML structure and css classes from the patternfly, but I do not have to import patternfly. Or should I also import patternfly into Ceph dashboard project?

@callithea
Copy link
Member

jenkins retest this please

@LenzGr LenzGr changed the title Remove top-right actions text and add "About" page mgr/dashboard: Remove top-right actions text and add "About" page Jun 29, 2018
@LenzGr LenzGr added the feature label Jun 29, 2018
@a2batic
Copy link
Contributor

a2batic commented Jun 29, 2018

@ricardoasmarques I had a doubt that we would be following patternfly library or patternfly-like modal after proposal. But, this looks good. I will test this and send my reviews.

@a2batic
Copy link
Contributor

a2batic commented Jun 30, 2018

jenkins retest this please

Copy link
Contributor

@a2batic a2batic left a comment

Choose a reason for hiding this comment

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

Lgtm

Copy link
Member

@callithea callithea left a comment

Choose a reason for hiding this comment

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

LGTM :)

constructor(public modalRef: BsModalRef, private summaryService: SummaryService) {}

ngOnInit() {
this.summaryService.get().subscribe((summary: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could remove a new request to the backend if, instead of calling get, we subscribe to the service.
We might need to replace Subject observable with BehaviorSubject, so we can obtain the value as soon as we subscribe.

outsideClick="true">
outsideClick="true"
i18n-title
title="Recent Notifications">
<i class="fa fa-bell"></i>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add fa-fw to all the icons on the navbar and remove the margin-right.
ATM they don't have the same size and spacing.

@tspmelo
Copy link
Contributor

tspmelo commented Jul 2, 2018

According to patternfly the label and version in the about modal should stay in the same line, but ceph version is too big and it automatically breaks.

Would it be ok to force a few more breaks? I think the result is more appealing to the eye:
about

We could also keep the 1st part in the 1st line:
image

@@ -0,0 +1,23 @@
.product-versions {
margin-bottom: 30px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be removed or reduced.

@LenzGr
Copy link
Contributor

LenzGr commented Jul 2, 2018

@tspmelo I'd be in favor of the second proposal (keep the actual version number in the same line as the "Ceph version" string and put the git commit hash and release code name underneath)

@a2batic
Copy link
Contributor

a2batic commented Jul 2, 2018

@tspmelo, I am also in favor of second one.

@epuertat
Copy link
Member

epuertat commented Jul 2, 2018

2nd as well. The only thing PF states about "about box" text is:

Content: Label and version. Adequate spacing and font weight consideration should be provided for legibility. Two columns are available for versions that contain both a release name and version number or in the event more space is needed.
We are utilizing the following:
When version and build information are both shown: Version 6.3 (Build 5)
When version only: Version 6.3
When build only: Build 5

Another approach would be splitting version into x.y.z, build and commit components through a pipe, but I don't think it's worthy.

BTW, this version string is taken at runtime from the ceph-mgr, right? So in case of upgrades, it will only change after the mgr/dashboard has been upgraded and restarted.

@ricardoasmarques ricardoasmarques force-pushed the wip-toolbar-text branch 3 times, most recently from c768178 to 92f86f1 Compare July 3, 2018 10:38
@ricardoasmarques
Copy link
Contributor Author

@tspmelo I've addressed your comments

Fixes: https://tracker.ceph.com/issues/24624

Signed-off-by: Ricardo Marques <rimarques@suse.com>
Signed-off-by: Ricardo Marques <rimarques@suse.com>
Fixes: https://tracker.ceph.com/issues/24646

Signed-off-by: Ricardo Marques <rimarques@suse.com>
@tspmelo
Copy link
Contributor

tspmelo commented Jul 4, 2018

jenkins retest this please

@LenzGr LenzGr merged commit bc1743e into ceph:master Jul 5, 2018
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