-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
Revamp the sign in and register pages #7872
Conversation
Worth posting this on community.Jenkins.io for feedback? |
I've posted it https://community.jenkins.io/t/an-updated-sign-in-and-register-page/6982 What are your thoughts/feedback? |
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.
I like the proposed design a lot, but I'm not a big fan of the new inline JS, such as https://github.com/jenkinsci/jenkins/pull/7872/files#diff-b1c3edf5abaac4d83594131e9e59e0ccdc259c4800b88cecd6e281f71ba4efb2R270, causing the need of follow-up PRs.
We generally avoid introducing new CSP violations.
Agreed, want me to split the JS as part of this PR? |
Preferably, if that is not too much work? At least the part you added 👀 |
Happy to do so 🙌 |
This looks like it's also addressing https://issues.jenkins.io/browse/JENKINS-71039, is that correct? |
Seems so, if this PR is merged, there's no inline JS left in |
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.
Tested manually, functionally seems alright to me.
Looks fine security wise too, thanks a lot for improving CSP situation!
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.
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! |
Causes JENKINS-71238. |
Also JENKINS-71253 |
Causes JENKINS-71246 |
Causes JENKINS-71238; therefore, adding the |
Sign in - Before
After
Register - Before
After
This PR introduces bolder, more visual sign in/register pages. They adopt the same starburst/gradient stylings from the About page and Design Library.
Also removes the inline JS in the register page, resolving https://issues.jenkins.io/browse/JENKINS-71039.
Testing done
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
@Restricted
or have@since TODO
Javadocs, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@jenkinsci/sig-ux
Maintainer checklist
Before the changes are marked as
ready-for-merge
:upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).