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

Use the Jenkins colour palette #161

Merged
merged 8 commits into from
May 7, 2024
Merged

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented May 6, 2024

Would be good to hear your thoughts on this @uhafner -

Currently the Prism plugin uses its default colour palette which differs from the Jenkins palette. This PR looks to set the default theme to follow Jenkins' palette instead. This brings a couple of advantages:

  • It looks more consistent with Jenkins
  • Jenkins themes don't need to provide a theme for Prism as long as they override the default colour palette (Dark theme does this already for example, so we could remove .withProperty("prism-api", "theme", PRISM_THEME) from that plugin)

Before

image image

After

image image (dark theme with the `prism-api` prop removed)

Testing done

Submitter checklist

Preview Give feedback

@uhafner uhafner added the enhancement Enhancement of existing functionality label May 7, 2024
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Thanks for updating the colors, it totally makes sense to provide a Jenkins theme.

@@ -8,7 +8,7 @@
* @author Ullrich Hafner
*/
public enum PrismTheme {
PRISM("Default (light and dark mode)", "prism.css"),
PRISM("Default (light and dark mode)", "default.css"),
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the assertion in the Test:

                .extracting(PrismTheme::getFileName)
                .isEqualTo("prism.css")

It actually does not verify anything useful and can be removed.

Copy link
Contributor Author

@janfaracik janfaracik May 7, 2024

Choose a reason for hiding this comment

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

Done - want the entire assertion removed or just those two lines? Thanks

@janfaracik janfaracik changed the title Use the Jenkins colour palette (for discussion) Use the Jenkins colour palette May 7, 2024
Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.39%. Comparing base (233dbfd) to head (ed0d489).
Report is 45 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #161      +/-   ##
============================================
- Coverage     75.59%   75.39%   -0.20%     
- Complexity       91       93       +2     
============================================
  Files            13       13              
  Lines           377      378       +1     
  Branches         30       30              
============================================
  Hits            285      285              
- Misses           89       90       +1     
  Partials          3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Thanks!

@uhafner uhafner merged commit 9f87b83 into jenkinsci:main May 7, 2024
26 of 27 checks passed
@janfaracik janfaracik deleted the new-colours branch May 7, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants