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 previews of the themes to the config page #75

Merged
merged 18 commits into from
Feb 23, 2022

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Nov 23, 2021

This PR intends to update the existing radio buttons on the configuration page to include previews of their respective theme.

Fixes #10

image

Related PRs:

TODO:

  • resource loading isn't updating on the fly in dark theme
  • Finish updating solarized theme
  • Do material theme
  • Enable caching in all themes and pass version number for cache busting - can be done later
  • Update screenshots in README
  • Verify compatibility on old themes

@timja
Copy link
Member

timja commented Nov 23, 2021

Ha was going to say that I think themes are too big, lots of scrolling once you get a few installed

screenshot

image

@timja
Copy link
Member

timja commented Nov 24, 2021

CSS is inlined in the Jelly - is this okay or is there a better, preferred approach for plugins?

No thanks 👅 .

There's two methods:

  1. package it as an adjunct, which is the simplest way I've done that in f763ba0 (#75)
  2. move the file to src/main/webapp and access it via jenkinsUrl/plugin/theme-manager/style.css

I'm not exactly sure the benefits of either tbh, @fqueiruga may have more experience with it

@timja
Copy link
Member

timja commented Nov 24, 2021

What would be the best way of pulling out CSS vars from the theme?

Tbh I think it's re-working themes so that they are all loaded on a page and name-spaced by a class. This would also allow hover / clicking to change the active theme on the page

(unless better ideas.)

Here's an example of how GitHub does themes:
https://github.githubassets.com/assets/dark_dimmed-92541d6f7b75e0061a44c901d2ff5bb9.css

[data-color-mode=light][data-light-theme=dark_dimmed], [data-color-mode=dark][data-dark-theme=dark_dimmed] {
...
}
@media (prefers-color-scheme: light) {
    [data-color-mode=auto][data-light-theme=dark_dimmed] {
...
}

They don't seem to automatically adjust svgs they just bundle a different SVG for each variation, but we might be able to make it automatic with a new namespacing.

We would need a flag on one of the classes that says the theme is namespacing aware / hot load capable.

Does this make sense? I can help if you need

@timja
Copy link
Member

timja commented Nov 24, 2021

Adding custom radios required a lot of custom HTML - this could be finicky if Jenkins changes how radios/JSON are saved

should be ok

@timja
Copy link
Member

timja commented Nov 24, 2021

For the record this branch only works with the update-forms one anyway, otherwise the checkboxes are broken.

One minor nit is that the checkbox looks a bit weird with a long label:

image

We could probably just shorten the label to System though

@timja
Copy link
Member

timja commented Nov 24, 2021

Test failure:

shouldn't be used because it doesn't work when the configuration item is repeated. Use to have your label attach to the previous DOM node instead.
url=file:/home/jenkins/workspace/ugins_theme-manager-plugin_PR-75/target/classes/io/jenkins/plugins/thememanager/ThemeManagerPageDecorator/global.jelly

bit weird, I guess just use attach-previous?

@timja
Copy link
Member

timja commented Nov 24, 2021

UX sig meeting is today at 4pm if you want to show this, (I've messaged you on teams)

@janfaracik janfaracik changed the title draft/discussion: Add previews of the themes to the config page Add previews of the themes to the config page Feb 16, 2022
@janfaracik janfaracik marked this pull request as ready for review February 16, 2022 23:09
@janfaracik janfaracik requested a review from a team as a code owner February 16, 2022 23:09
Co-authored-by: Ullrich Hafner <ullrich.hafner@gmail.com>
pom.xml Outdated Show resolved Hide resolved
@timja timja added the enhancement New feature or request label Feb 20, 2022
@timja
Copy link
Member

timja commented Feb 20, 2022

Theme manager side of this is ready to go I think.

Edit: moved TODO to top comment

@timja
Copy link
Member

timja commented Feb 22, 2022

resource loading isn't updating on the fly in dark theme

Bisecting in progress, tracked it down to a parent pom regression, most likely in maven-hpi-plugin.

4.10 - good
4.22 - good
4.24 - good
4.25 - good
4.26 - bad
4.29 - bad
4.35 - bad

= 4.26 - bad

Bisected parent pom:
jenkinsci/plugin-pom#436

commit 80a8d32c80d2591b8c3b8d3cfa65312f8f44c546
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Sep 13 08:06:40 2021 +0100

    Bump maven-hpi-plugin from 3.17 to 3.18 (#436)

    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bisected to jenkinsci/maven-hpi-plugin#208

@timja
Copy link
Member

timja commented Feb 23, 2022

Just need README screenshot update now I think

@janfaracik
Copy link
Contributor Author

Just need README screenshot update now I think

Done 👍

image

pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

🎉 let's go!

@timja timja merged commit 77e23a9 into jenkinsci:master Feb 23, 2022
@janfaracik janfaracik deleted the new-radios branch February 23, 2022 22:23
@NotMyFault
Copy link
Member

NotMyFault commented Feb 23, 2022

Nice to see it finally go live 🥳

But why does it just deploy to incrementals' repository?

@timja
Copy link
Member

timja commented Feb 24, 2022

What do you mean? I’ll ship dark theme soon and hopefully others will ship the rest


There was an error during release because of the GitHub action reformatting the code on master: #84

When I resumed the release it built 0.7-SNAPSHOT instead of actually releasing it 🤦

@NotMyFault
Copy link
Member

Yeah, I noticed it not being available thru the plugin manager after the tag has been published.
But thanks for re-releasing it 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Theme Preview
4 participants