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

Add Appearance system configuration page #8403

Merged
merged 4 commits into from
Aug 29, 2023

Conversation

timja
Copy link
Member

@timja timja commented Aug 21, 2023

This adds a new system configuration page 'Appearance' for configuring the look and feel of Jenkins.
It moves a bit of config out of the main System page into a more appropriate place.

Screenshots image

image

image

image

It implements the design from https://jenkins-redesign.vercel.app/settings/appearance

It complements #8379 but doesn't require it

I've gone through a number of plugins that contribute UI and have sent pull requests for:

TODO

Preview Give feedback
  1. 6 of 6
    enhancement
    TobiX
  2. 6 of 6
    enhancement
  3. 6 of 6
    breaking enhancement
  4. 6 of 6
    enhancement
    uhafner

I have gone for ones that directly affect the look and feel of Jenkins rather than ones that affect builds like ansicolor.
Any other suggestions?

After #7268 is in the user profile page can be updated to match.

Could possibly move the Display notification URL provider to appearance too? (the thing that selects Classic, Blue Ocean or Pipeline Graph View)

Testing done

Started Jenkins with no plugins contributing to the appearance category, the page doesn't show.

Installed jenkinsci/theme-manager-plugin#210, the page shows up and theme manager is on the appearance page and not on the system configuration page.

Unit test wise I don't think there's much value for it will something in core uses it, although happy to add something if its thought to be needed

Proposed changelog entries

  • Add appearance system configuration page

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

@timja timja force-pushed the apperance-extension-point branch from eb29185 to 3e12ca2 Compare August 21, 2023 07:25
@timja timja added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise labels Aug 22, 2023
@TobiX
Copy link
Contributor

TobiX commented Aug 22, 2023

Nice. https://plugins.jenkins.io/test-results-analyzer/ looks like an additional candidate, but mixes appearance settings with other settings...

@Kevin-CB Kevin-CB added the security-approved @jenkinsci/core-security-review reviewed this PR for security issues label Aug 24, 2023
@Kevin-CB
Copy link
Contributor

From a security perspective, I see no blockers!

@timja
Copy link
Member Author

timja commented Aug 24, 2023

Nice. plugins.jenkins.io/test-results-analyzer looks like an additional candidate, but mixes appearance settings with other settings...

I've had a look but I don't think it fits with the page that well.
I'm trying to have it be more for things that affect look and feel as a whole rather than some niche plugin config

@timja timja marked this pull request as ready for review August 24, 2023 21:48
@timja timja requested a review from a team August 24, 2023 21:51
@janfaracik
Copy link
Contributor

I think this is great! LGTM.

Would be good to see something similar in #7268 if it's going forward.

@timja timja requested a review from a team August 25, 2023 20:04
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

That's awesome!

@timja
Copy link
Member Author

timja commented Aug 28, 2023

/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!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Aug 28, 2023
@daniel-beck
Copy link
Member

daniel-beck commented Aug 28, 2023

It moves a bit of config out of the main System page into a more appropriate place.

Previously, creation of new config categories were motivated by overcrowding on the global configuration (Tools, Clouds, possibly Security as well – matrix-auth gets huge quickly). This doesn't seem to be the case here. What motivates this change, what's special about this config page that does not apply to tons of other potential categories?

Also, we should better define what goes here so plugin maintainers know what's appropriate to put there. The discussion about some plugins above indicates this isn't the case.

@timja
Copy link
Member Author

timja commented Aug 28, 2023

Previously, creation of new config categories were motivated by overcrowding on the global configuration (Tools, Clouds, possibly Security as well – matrix-auth gets huge quickly). This doesn't seem to be the case here. What motivates this change, what's special about this config page that does not apply to tons of other potential categories?

This groups related elements together, originally I had started jenkinsci/theme-manager-plugin#105 but that would've been a breaking change at configuration level. This pull request #8379 then reminded of the design https://jenkins-redesign.vercel.app/settings/appearance. Which seemed like a much cleaner approach to solve the problem.

It means that you can configure the look and feel of Jenkins from one place without having to scroll all over the place to find it.

what's special about this config page that does not apply to tons of other potential categories

Other sensible categories could likely be added if there are enough sensible elements that would go there? e.g. possibly notifications?

Also, we should better define what goes here so plugin maintainers know what's appropriate to put there. The discussion about some plugins above indicates this isn't the case.

22b858b (#8403)

@daniel-beck
Copy link
Member

Other sensible categories could likely be added if there are enough sensible elements that would go there? e.g. possibly notifications?

That makes sense if we move from "this is unwieldy, split it up" to "categories are based on loosely coupled semantic relationships between elements". That seems like a JEP-level change though given the downstream impact.

@timja
Copy link
Member Author

timja commented Aug 28, 2023

Other sensible categories could likely be added if there are enough sensible elements that would go there? e.g. possibly notifications?

That makes sense if we move from "this is unwieldy, split it up" to "categories are based on loosely coupled semantic relationships between elements". That seems like a JEP-level change though given the downstream impact.

The javadoc already says that as far as I can tell

* Grouping of related {@link GlobalConfiguration}s.

@timja timja merged commit d7da362 into jenkinsci:master Aug 29, 2023
@timja timja deleted the apperance-extension-point branch August 29, 2023 08:32
@jonesbusy
Copy link
Contributor

Looks login-theme-plugin was forgotten.

I've open a PR here : jenkinsci/login-theme-plugin#37

Regards,

@KalleOlaviNiemitalo
Copy link

On the System configuration page, I see a few settings that could perhaps be moved to Appearance:

  • "System Message" (Jenkins.systemMessage). Especially because the Customizable Header plugin has its own "System Message" setting on the Appearance page.
  • "ANSI Color": was already rejected above.
  • "Additional Sidebar Links" (Sidebar Link plugin): Didn't find an existing issue for this.
  • "Prism Syntax Highlighting Configuration" (Prism API plugin): Didn't find an existing issue for this.

If there's no objection, I think I'll file issues for the last two in a few days.

@uhafner
Copy link
Member

uhafner commented Nov 21, 2023

  • "Prism Syntax Highlighting Configuration" (Prism API plugin): Didn't find an existing issue for this.

https://github.com/jenkinsci/prism-api-plugin/releases/tag/v1.29.0-9

@jonesbusy
Copy link
Contributor

"Additional Sidebar Links" (Sidebar Link plugin): Didn't find an existing issue for this.

As a user of sidebar-link I think it should be moved to appearance as well. But you should check with maintainer. Not sure if there is "guidelines" of what is appearance and what is not

@timja
Copy link
Member Author

timja commented Nov 21, 2023

"Additional Sidebar Links" (Sidebar Link plugin): Didn't find an existing issue for this.

As a user of sidebar-link I think it should be moved to appearance as well. But you should check with maintainer. Not sure if there is "guidelines" of what is appearance and what is not

Yeah it makes sense for that to go there, https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/jenkins/appearance/AppearanceCategory.java#L30-L41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants