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

Jump to bottom

mgr/dashboard: customizable log-in page text/banner #45951

Merged
merged 1 commit into from May 19, 2022
Merged

mgr/dashboard: customizable log-in page text/banner #45951

merged 1 commit into from May 19, 2022

Conversation

Sarthak0702
Copy link
Member

@Sarthak0702 Sarthak0702 commented Apr 19, 2022 •
edited

Three new CLI commands added to module.py:

  • ceph dashboard set-login-banner -i <filename>
  • ceph dashboard get-login-banner
  • ceph dashboard unset-login-banner

Before:
image

After:
image

Fixes:https://tracker.ceph.com/issues/55231
Signed-off-by: Sarthak0702 sarthak.dev.0702@gmail.com

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@Sarthak0702 Sarthak0702 requested a review from a team as a code owner April 19, 2022 08:32
@Sarthak0702 Sarthak0702 requested review from avanthakkar and nizamial09 and removed request for a team April 19, 2022 08:32
@Sarthak0702 Sarthak0702 added this to In progress in Dashboard via automation Apr 19, 2022
@Sarthak0702
Copy link
Member Author

jenkins test make check

@epuertat
Copy link
Member

@Sarthak0702 :

************* Module controllers.home
controllers/home.py:15:0: C0411: third party import "import cherrypy" should be placed before "from ..services.custom_banner import get_custom_login_banner_file" (wrong-import-order)
controllers/home.py:16:0: C0411: third party import "from cherrypy.lib.static import serve_file" should be placed before "from ..services.custom_banner import get_custom_login_banner_file" (wrong-import-order)
************* Module services.custom_banner
services/custom_banner.py:10:4: W1203: Use lazy % formatting in logging functions (logging-fstring-interpolation)

epuertat
epuertat reviewed Apr 19, 2022
View reviewed changes
Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

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

Nice quick work!

Regarding the placement, I'd put that above the input fields (as in the OpenStack UI):

image

Our Patternfly fellows recommend this instead:

image

But IMHO unless you put a logo above that the text feels a bit misplaced....

src/pybind/mgr/dashboard/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/module.py Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/services/custom_banner.py Outdated Show resolved Hide resolved
@Sarthak0702 Sarthak0702 force-pushed the feature-set-login-banner branch 2 times, most recently from 7838223 to 9d70c39 Compare April 20, 2022 07:40
nizamial09
nizamial09 previously requested changes Apr 25, 2022
View reviewed changes
Copy link
Member

@nizamial09 nizamial09 left a comment

Choose a reason for hiding this comment

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

Great work so far @Sarthak0702. Couple of concerns from my side:

Should this change the alignment of the login page components like this?

Screenshot from 2022-04-25 13-47-08

Without the login-banner everything is properly aligned, but the banner misaligns it.
Screenshot from 2022-04-25 13-44-53

I think the banner should be visible without causing much change to alignment of the existing items.

src/pybind/mgr/dashboard/frontend/src/app/core/layouts/login-layout/login-layout.component.html Outdated Show resolved Hide resolved
...rd/frontend/src/app/shared/components/custom-login-banner/custom-login-banner.component.scss Outdated Show resolved Hide resolved
...oard/frontend/src/app/shared/components/custom-login-banner/custom-login-banner.component.ts Outdated Show resolved Hide resolved
...oard/frontend/src/app/shared/components/custom-login-banner/custom-login-banner.component.ts Outdated Show resolved Hide resolved
...oard/frontend/src/app/shared/components/custom-login-banner/custom-login-banner.component.ts Outdated Show resolved Hide resolved
...oard/frontend/src/app/shared/components/custom-login-banner/custom-login-banner.component.ts Outdated Show resolved Hide resolved
src/pybind/mgr/dashboard/module.py Show resolved Hide resolved
Dashboard automation moved this from In progress to Review in progress Apr 25, 2022
@Sarthak0702
Copy link
Member Author

I think the banner should be visible without causing much change to alignment of the existing items.

@nizamial09 Yup, this does change the alignment of the login page components. Aligning them with the login text is a bit tricky.

@Sarthak0702
Copy link
Member Author

jenkins test api

@Sarthak0702
Copy link
Member Author

jenkins test dashboard

2 similar comments
@Sarthak0702
Copy link
Member Author

jenkins test dashboard

@Sarthak0702
Copy link
Member Author

jenkins test dashboard

@Sarthak0702 Sarthak0702 requested a review from pereman2 May 10, 2022 13:20
@Sarthak0702 Sarthak0702 assigned aaSharma14 and unassigned aaSharma14 May 10, 2022
@Sarthak0702
Copy link
Member Author

jenkins test dashboard

@Sarthak0702 Sarthak0702 changed the title mgr/dashboard: customizable log-in page text/banner mgr/dashboard: customizable log-in page text/ banner May 11, 2022
@Sarthak0702 Sarthak0702 changed the title mgr/dashboard: customizable log-in page text/ banner mgr/dashboard: customizable log-in page text/banner May 11, 2022
@Sarthak0702
Copy link
Member Author

jenkins retest this please

@Sarthak0702
Copy link
Member Author

jenkins retest this please

@Sarthak0702
Copy link
Member Author

make check failed due to:

[lint:html    ] src/app/shared/components/custom-login-banner/custom-login-banner.component.html: line 2, col 17, ids and classes may not use the word: banner

@epuertat any idea why is this check needed?

@Sarthak0702
Copy link
Member Author

jenkins test dashboard cephadm

@Sarthak0702
Copy link
Member Author

jenkins test dashboard

@Sarthak0702
mgr/dashboard: customizable log-in page text/banner
9f8bcd7 
Fixes:https://tracker.ceph.com/issues/55231
Signed-off-by: Sarthak0702 <sarthak.dev.0702@gmail.com>
@Sarthak0702
Copy link
Member Author

jenkins test dashboard cephadm

Dashboard automation moved this from Review in progress to Reviewer approved May 16, 2022
@avanthakkar
Copy link
Contributor

jenkins test dashboard

avanthakkar
avanthakkar approved these changes May 17, 2022
View reviewed changes
Copy link
Contributor

@avanthakkar avanthakkar left a comment

Choose a reason for hiding this comment

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

LGTM! Good work @Sarthak0702

@Sarthak0702
Copy link
Member Author

jenkins test dashboard

@epuertat
Copy link
Member

This PR will be causing the following layout change (login form shifted upwards):

image

Are we ok with this change? Is there any way @Sarthak0702 to only displace these elements when the banner text is defined (or not displace anything and just leave the banner text uncentered)?

Sarthak0702
Sarthak0702 commented May 18, 2022
View reviewed changes
src/pybind/mgr/dashboard/frontend/src/app/core/layouts/login-layout/login-layout.component.html
Comment on lines +12 to +19
<div class="row full-height">
<div class="col-sm-12 col-md-6 d-sm-block login-form">
<router-outlet></router-outlet>
</div>
<div class="col-sm-12 col-md-6 d-sm-block">
<div class="col-sm-12 col-md-6 d-sm-block branding-info">
<img src="assets/Ceph_Ceph_Logo_with_text_white.svg"
alt="Ceph"
class="img-fluid">
class="img-fluid pb-3">
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any way @Sarthak0702 to only displace these elements when the banner text is defined (or not displace anything and just leave the banner text uncentered)?

@epuertat If we want that then classes like vertical-align, login-form and branding-info will need to be added conditionally based on if bannerText from custom-login-component exists of not. Unless we really want it to be centered and are not changing code just to pass test I guess its should be possible (but a bit tricky/hacky).

Copy link
Member

Choose a reason for hiding this comment

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

Ok, not worthy. Let's get this merged. Probably we should have explored absolute positioning or a different approach, but definitely not worthy :P

@epuertat epuertat moved this from Reviewer approved to Ready-to-merge in Dashboard May 19, 2022
@epuertat
Copy link
Member

Only failed due to visual regression caused by login layout changes.

@epuertat epuertat merged commit 4edef77 into ceph:master May 19, 2022
12 of 13 checks passed
Dashboard automation moved this from Ready-to-merge to Done May 19, 2022
This was referenced May 19, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epuertat epuertat epuertat approved these changes

@avanthakkar avanthakkar avanthakkar approved these changes

@nizamial09 nizamial09 Awaiting requested review from nizamial09

@pereman2 pereman2 Awaiting requested review from pereman2

@aaSharma14 aaSharma14 Awaiting requested review from aaSharma14

Assignees
No one assigned
Labels
dashboard needs-review pybind
Projects
Ceph-Dashboard
Archived in project
Dashboard
  
Done
Milestone
No milestone
5 participants
@Sarthak0702 @epuertat @avanthakkar @nizamial09 @aaSharma14