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 CSS variables - prerequisite for Dark mode #4752

Merged
merged 31 commits into from
May 31, 2020
Merged

Conversation

timja
Copy link
Member

@timja timja commented May 27, 2020

See JENKINS-XXXXX.

This is being used to deliver dark mode, see https://github.com/jenkinsci/dark-theme

Testing

It would be great if someone can check this in IE 11, CSS variables aren't supported there but I've added a postcss plugin that has resolved the variables at build time, I've checked the properties are present in the CSS.

docker run --rm -ti -p 8080:8080 -e ID=4752 jenkins/core-pr-tester

Proposed changelog entries

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

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

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@res0nance
Copy link
Contributor

I was going to test this on windows but with the current tip, it doesnt work on windows

[INFO] $ webpack --config webpack.config.js --mode=production
[ERROR] Browserslist: caniuse-lite is outdated. Please run next command `yarn upgrade`
[ERROR] webpack-fix-style-only-entries: removing js from style only module: base-styles-v2.js
[ERROR] webpack-fix-style-only-entries: removing js from style only module: ui-refresh-overrides.js
[INFO] Hash: 2b2ca561afb45798718d
[INFO] Version: webpack 4.41.2
[INFO] Time: 19069ms
[INFO] Built at: 05/27/2020 10:58:08 PM
[INFO]  51 assets
[INFO] Entrypoint page-init = vendors.js page-init.js
[INFO] Entrypoint pluginSetupWizard = vendors.js pluginSetupWizard.css pluginSetupWizard.js
[INFO] Entrypoint upgradeWizard = vendors.js upgradeWizard.js
[INFO] Entrypoint add-item = vendors.js add-item.css add-item.js
[INFO] Entrypoint config-scrollspy = vendors.js config-scrollspy.css config-scrollspy.js
[INFO] Entrypoint config-tabbar = vendors.js config-tabbar.css config-tabbar.js
[INFO] Entrypoint base-styles-v2 = vendors.js base-styles-v2.css
[INFO] Entrypoint ui-refresh-overrides = vendors.js
[INFO] [31] multi ./src/main/js/page-init.js 28 bytes {5} [built]
[INFO] [32] ./src/main/js/page-init.js 910 bytes {5} [built]
[INFO] [35] multi ./src/main/js/pluginSetupWizard.js ./src/main/less/pluginSetupWizard.less 40 bytes {6} [built]
[INFO] [41] ./src/main/less/pluginSetupWizard.less 711 bytes {6} [built]
[INFO] [43] multi ./src/main/js/upgradeWizard.js 28 bytes {8} [built]
[INFO] [44] ./src/main/js/upgradeWizard.js 1.4 KiB {8} [built]
[INFO] [45] multi ./src/main/js/add-item.js ./src/main/js/add-item.less 40 bytes {1} [built]
[INFO] [46] ./src/main/js/add-item.js 9.96 KiB {1} [built]
[INFO] [47] ./src/main/js/add-item.less 702 bytes {1} [built]
[INFO] [49] multi ./src/main/js/config-scrollspy.js ./src/main/js/config-scrollspy.less 40 bytes {3} [built]
[INFO] [50] ./src/main/js/config-scrollspy.js 4.54 KiB {3} [built]
[INFO] [51] ./src/main/js/config-scrollspy.less 710 bytes {3} [built]
[INFO] [53] multi ./src/main/js/config-tabbar.js ./src/main/js/config-tabbar.less 40 bytes {4} [built]
[INFO] [57] multi ./src/main/less/base-styles-v2.less 28 bytes {2} [built]
[INFO] [60] multi ./src/main/less/ui-refresh-overrides.less 28 bytes {7} [built]
[ERROR] error Command failed with exit code 2.
[INFO]     + 57 hidden modules
[INFO]
[INFO] ERROR in ./src/main/less/ui-refresh-overrides.less (./node_modules/mini-css-extract-plugin/dist/loader.js!./node_modules/css-loader/dist/cjs.js??ref--4-2!./node_modules/postcss-loader/src!./node_modules/less-loader/dist/cjs.js!./src/main/less/ui-refresh-overrides.less)
[INFO] Module build failed (from ./node_modules/mini-css-extract-plugin/dist/loader.js):
[INFO] ModuleNotFoundError: Module not found: Error: Can't resolve './abstracts/menu_down_arrow.png' in 'D:\github\jenkins\war\src\main\less'
[ERROR] error Command failed with exit code 2.

@@ -109,7 +109,7 @@

#menuSelector {/* used for showing 'v' on the right of the anchor */
background-color:transparent;
background-image: url(menu_down_arrow.png);
background-image: var(--menu-selector-down-background-image);
Copy link
Member Author

Choose a reason for hiding this comment

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

anyone able to check if this works on IE 11?

image

I suspect that it needs a manual fallback, looks like the postcss processor isn't adding the fallback for it

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to work on i.e but the entire dark theme doesn't

Here is a side by side with ie11 on the left and edge on the right

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to work on i.e but the entire dark theme doesn't

It's not expected to work on IE 11 at all for the theme

Here is a side by side with ie11 on the left and edge on the right

That's not working, at least for fonts, the preprocessor isn't picking them up 😢,

Two options I guess,

  1. this work doesn't really require the fonts to be changeable at runtime, so it could go back to less variables
  2. try out the other postcss plugin and see if that handles fonts, this one only seems to handle colours

cc @fqueiruga

Copy link
Contributor

Choose a reason for hiding this comment

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

Some more observations I made:

  • The administrative monitor (The bell thing in the top right) still shows white background on the dark theme
  • CLI page has black text with dark theme.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks I’d forgotten about the admin monitor issue, and I hadn’t checked the CLI page

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW I would not bother converting the arrow to a CSS variables. It should be either an icon or a CSS triangle, not an image anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

@timja timja added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label May 27, 2020
@fqueiruga
Copy link
Contributor

I don't think we should go all in CSS variables. For example, I would keep font-family and font-size in less variables for now.

In the first iteration I'd keep it simple: colors, backgrounds, borders and maybe font weights.

@line-height-heading: 1.2;
body {
// Font related properties
--font-family-sans: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, "Noto Sans", Oxygen, Ubuntu, Cantarell, "Fira Sans", "Droid Sans", "Helvetica Neue", Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol";
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the fact that the variables are declared here and not in :root is causing the fallback issues. Just some brainstorming.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes you’re right

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just go ahead and create a big & comprehensive variables.less file and be done with it :D

@@ -3,7 +3,7 @@

.ui-refresh {
.page-header {
background-color: @header-bg-v2;
background-color: var(--header-bg-v2);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if we should just override the :root variable here

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm? not quite sure what you mean

war/src/main/less/pluginSetupWizard.less Outdated Show resolved Hide resolved
war/src/main/less/modules/side-panel-tasks.less Outdated Show resolved Hide resolved
@line-height-heading: 1.2;
body {
// Font related properties
--font-family-sans: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, "Noto Sans", Oxygen, Ubuntu, Cantarell, "Fira Sans", "Droid Sans", "Helvetica Neue", Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol";
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just go ahead and create a big & comprehensive variables.less file and be done with it :D

war/src/main/less/abstracts/colors.less Show resolved Hide resolved
war/postcss.config.js Show resolved Hide resolved
box-shadow: var(--menu-box-shadow);
}

#jenkins.yui-skin-sam {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you need to create this? Does this fix something?

Copy link
Member Author

Choose a reason for hiding this comment

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

it allows overriding the yui-skin in themes, I've matched the css below to what the yui skin provides and extended it with variables

@@ -109,7 +109,7 @@

#menuSelector {/* used for showing 'v' on the right of the anchor */
background-color:transparent;
background-image: url(menu_down_arrow.png);
background-image: var(--menu-selector-down-background-image);
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW I would not bother converting the arrow to a CSS variables. It should be either an icon or a CSS triangle, not an image anyway.

@oleg-nenashev
Copy link
Member

@fqueiruga
Copy link
Contributor

There are some variables that use -background- and some that use -bg-. I think we should standardize on one. I'm partial towards -bg- but I'm not really invested on it.

@timja
Copy link
Member Author

timja commented May 28, 2020

There are some variables that use -background- and some that use -bg-. I think we should standardize on one. I'm partial towards -bg- but I'm not really invested on it.

Agreed, most of the bg ones came from the existing sass variables and I didn't touch them in the find / replace,

When I created new ones I added them with background, happy to shorten though

@fqueiruga
Copy link
Contributor

Created a PR against this branch timja#3

@fqueiruga
Copy link
Contributor

Another PR created against this branch timja#4

@fqueiruga
Copy link
Contributor

Created another PR, forgot one variable timja#6 😓

@fqueiruga
Copy link
Contributor

fqueiruga commented May 28, 2020

I'll start working on the dark mode variables for these changes

@timja
Copy link
Member Author

timja commented May 29, 2020

I've updated the changelog PTAL

@oleg-nenashev oleg-nenashev requested review from a team May 29, 2020 12:36
Copy link
Contributor

@res0nance res0nance left a comment

Choose a reason for hiding this comment

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

Retested up to c12b42c
With IE11 things look ok there

@timja
Copy link
Member Author

timja commented May 29, 2020

Some shuffling done in 49e501f mostly moves yui customisation code to one place, as there was some in styles.less as well

I removed a bit of duplication between the search bar and other autocompletes, and pulled in some of the enhancements that were added to search but not others.

I've tested it on the add job, copy items from widget

@oleg-nenashev
Copy link
Member

We may merge it in 24 hours if no negative feedback or requests to extend the review time. Please see the merge process documentation for more information

Thanks to @timja @95jonpet @fqueiruga @res0nance @EstherAF @nimishbongale who contributed to the Dark Theme implementation and evaluation, and to compatibility testing of this pull request! It is great to see such a major improvement to Jenkins UX and CSS extensibility delivered in a few days. 🥇 🙇 🚀

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label May 29, 2020
@oleg-nenashev
Copy link
Member

jenkinsci/dark-theme-plugin#60 updates the demo for testing to the recent version

@halkeye
Copy link
Member

halkeye commented May 30, 2020

so not a blocker but a few concerns:

  1. Sometimes the fallback values before the variables are kept like
    https://github.com/jenkinsci/jenkins/pull/4752/files#diff-21e1a8d53a5c4282d7a7c221551cf7f9L22-R24
    But other times they are not
    Since the postcss plugin should replace the variables at compile time, i don't think the fallbacks are necessary and will get out of sync/confusion quickly
  2. I haven't checked but does the plugin keep the variables in the final output? I would assume so because they would make themeing so much easier. If not, can it be configured to do the replacements and keep them?

@timja
Copy link
Member Author

timja commented May 30, 2020

  1. The manual fallbacks are kept in the adjuncts which aren’t passed through webpack / postcss
  2. Replied to this in a different comment but the fallbacks are added above the variables in the final output, and are only used if a browser doesn’t support variables (Internet explorer)

// Side panel
--panel-border-color: #eaeff2;
--panel-border-color--hover: #cecece;
--side-panel-hover-color: var(--panel-border-color);
Copy link
Member

Choose a reason for hiding this comment

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

Defined but never used, also naming inconsistent with other variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be removed IMO

border-color: @btn-primary-bg;
color: var(--btn-text-color);
background-color: var(--btn-primary-bg);
border-color: var(--btn-primary-bg);
Copy link
Member

Choose a reason for hiding this comment

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

Is it deliberate that here and further below, we're not using a separate variable for the primary button border? Secondary buttons have a separate variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

historical most likely, not intentional as far as I know

Copy link
Contributor

Choose a reason for hiding this comment

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

Button variable API should be expanded IMO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal major-rfe For changelog: Major enhancement. Will be highlighted on the top ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback squash-merge-me Unclean or useless commit history, should be merged only with squash-merge 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.

8 participants