-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
Make branding overridable in signin/login page #8462
Make branding overridable in signin/login page #8462
Conversation
* Move resources from DefaultSimplePageDecorator to SimplePageDecorator to ease implementation.
This demonstrates branding of the login/register screen made possible via jenkinsci/jenkins#8462
@@ -48,19 +48,16 @@ THE SOFTWARE. | |||
<!-- mobile friendly layout --> | |||
<meta name="viewport" content="width=device-width, initial-scale=1"/> | |||
<!-- css styling, will fallback to default implementation --> | |||
<st:include it="${simpleDecorator}" page="simple-head.jelly" optional="true"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove these optionals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the views are moved from DefaultSimplePageDecorator
to SimplePageDecorator
now, they can't be missing, so just clarifying that (although it doesn't change anything functionally)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this with theme manager and a theme? i.e. is it going to break because simple-branding
is now required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine as the default from SimplePageDecorator
will be used if none is provided by the concrete implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not tested manually but looks good, thanks!
/label ready-for-merge This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback. Thanks! |
This makes branding implementation in login/register pages overridable via a
SimplePageDecorator
implementation.cc @dwnusbaum
Testing done
Proposed changelog entries
SimplePageDecorator
.Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
Before the changes are marked as
ready-for-merge
:Maintainer checklist