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

Create Color Definition for Reuse #2455

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BeckerWdf
Copy link
Contributor

@BeckerWdf BeckerWdf commented Oct 28, 2024

Instead of writing the same RBG color codes over and over again in various places in the CSS files only do it once.
Define a new color and re-use that color definition at all other locations. This also adds a semantic name to the color code.

Unfortunately color definitions cannot refer to other color definitions. So at some places we still have to provides the color code. These places are marked with a comment.

The new defined color is now also visible in the "Colors and Fonts" preference page. I am unsure if this is something we want. If users change this this does not work 100% because of the limitation above.

@BeckerWdf
Copy link
Contributor Author

I only tested on macOS.

@BeckerWdf
Copy link
Contributor Author

@mvm-sap, @sratz what do you think?

Copy link
Contributor

github-actions bot commented Oct 28, 2024

Test Results

 1 214 files  ±0   1 214 suites  ±0   1h 14m 53s ⏱️ + 1m 57s
 7 729 tests ±0   7 494 ✅  - 1  233 💤 ±0  2 ❌ +1 
16 232 runs  ±0  15 715 ✅  - 1  515 💤 ±0  2 ❌ +1 

For more details on these failures, see this check.

Results for commit f59a57c. ± Comparison against base commit 8bd169c.

♻️ This comment has been updated with latest results.

@BeckerWdf
Copy link
Contributor Author

The new defined color is now also visible in the "Colors and Fonts" preference page. I am unsure if this is something we want. If users change this this does not work 100% because of the limitation above.

With 845377a this issue is fixed.

@eclipse-platform-bot
Copy link
Contributor

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

bundles/org.eclipse.e4.ui.css.swt/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch
From 539b2270bdd6afe6a47f90f107b2d14cf738df63 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Tue, 29 Oct 2024 09:35:16 +0000
Subject: [PATCH] Version bump(s) for 4.34 stream


diff --git a/bundles/org.eclipse.e4.ui.css.swt/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.ui.css.swt/META-INF/MANIFEST.MF
index a10944e052..b6e6c4e55d 100644
--- a/bundles/org.eclipse.e4.ui.css.swt/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.e4.ui.css.swt/META-INF/MANIFEST.MF
@@ -1,7 +1,7 @@
 Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-SymbolicName: org.eclipse.e4.ui.css.swt;singleton:=true
-Bundle-Version: 0.15.400.qualifier
+Bundle-Version: 0.15.500.qualifier
 Bundle-Name: %pluginName
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
-- 
2.47.0

Further information are available in Common Build Issues - Missing version increments.

Copy link
Member

@sratz sratz left a comment

Choose a reason for hiding this comment

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

Are we somehow committing to the id of the definition org-eclipse-ui-workbench-SECONDARY_BACKGROUND and have to keep it stable?

I guess with the latest hidden flag, we can consider this ID to be purely internal and should be able to rename this?

bundles/org.eclipse.ui.themes/css/e4_preview_gtk.css Outdated Show resolved Hide resolved
@BeckerWdf
Copy link
Contributor Author

Are we somehow committing to the id of the definition org-eclipse-ui-workbench-SECONDARY_BACKGROUND and have to keep it stable?

I guess with the latest hidden flag, we can consider this ID to be purely internal and should be able to rename this?

I would say we can consider it internal as it only appears in the CSS files. Anyway: Do you have a better name?

@sratz
Copy link
Member

sratz commented Oct 29, 2024

I would say we can consider it internal as it only appears in the CSS files.

👍

Anyway: Do you have a better name?

No, hence the question 😉

@vogella
Copy link
Contributor

vogella commented Oct 29, 2024

I think it is great to allow the user to change the default colour. I think it should be relatively easy to implement that colors refer to others in CSS. What exactly is the requirement here @BeckerWdf ?

@BeckerWdf
Copy link
Contributor Author

BeckerWdf commented Oct 29, 2024

think it should be relatively easy to implement that colors refer to others in CSS. What exactly is the requirement here?

What I want to do is this:

ColorDefinition#org-eclipse-ui-workbench-ACTIVE_UNSELECTED_TABS_COLOR_START {
	color: '#org-eclipse-ui-workbench-SECONDARY_BACKGROUND';
}

instead of:

ColorDefinition#org-eclipse-ui-workbench-ACTIVE_UNSELECTED_TABS_COLOR_START {
	color: #F8F8F8;
}

@vogella: Would be great if you could help here. Feel free to push a separate PR / an additional commit to this PR

@BeckerWdf
Copy link
Contributor Author

This PR needs to be reworked after #2549 is merged.

@BeckerWdf
Copy link
Contributor Author

pr-head fails with:

12:20:08.576 [ERROR] Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.10:compare-version-with-baselines (compare-attached-artifacts-with-release) on project org.eclipse.jface: Baseline and reactor have the same fully qualified version, but different content

@HannesWell: What's the issue here?

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

Successfully merging this pull request may close these issues.

4 participants