Skip to content

Conversation

@mvm-sap
Copy link
Contributor

@mvm-sap mvm-sap commented Dec 19, 2024

With the dark theme changes, selected sub-tabs in the editor and views were missing blue highlights. This has been fixed with this PR. The screenshots below are from Windows. The same change has been incorporated in Mac and Linux.

Before:
Screenshot 2024-12-19 231102

After:
Screenshot 2024-12-19 225628

@github-actions
Copy link
Contributor

github-actions bot commented Dec 19, 2024

Test Results

 1 821 files  ±0   1 821 suites  ±0   1h 34m 43s ⏱️ + 2m 35s
 7 734 tests ±0   7 506 ✅ +1  228 💤 ±0  0 ❌ ±0 
24 363 runs  ±0  23 614 ✅ +1  749 💤 ±0  0 ❌ ±0 

Results for commit 496e376. ± Comparison against base commit 2de3957.

♻️ This comment has been updated with latest results.

Copy link
Member

@BeckerWdf BeckerWdf left a comment

Choose a reason for hiding this comment

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

I manually tested this. Works fine an fixes the bug.

I see that the changes are the same on all 3 platforms (CSS rules are identical).
Can't we then simply move this into the e4-dark_globalstyle.css only once instead of duplicating it three times?

@mvm-sap
Copy link
Contributor Author

mvm-sap commented Dec 23, 2024

I manually tested this. Works fine an fixes the bug.

I see that the changes are the same on all 3 platforms (CSS rules are identical). Can't we then simply move this into the e4-dark_globalstyle.css only once instead of duplicating it three times?

Okay, I shall try to move and test again

@BeckerWdf
Copy link
Member

BeckerWdf commented Dec 23, 2024

I manually tested this. Works fine an fixes the bug.

I see that the changes are the same on all 3 platforms (CSS rules are identical). Can't we then simply move this into the e4-dark_globalstyle.css only once instead of duplicating it three times?

or in e4-dark_tabstyle.css

@mvm-sap mvm-sap force-pushed the Dark_Theme_Sub_Tab_Highlight_Issue_Fix branch from fe43009 to f18b3fa Compare December 24, 2024 04:47
@BeckerWdf
Copy link
Member

The current state of the PR does not work correctly on macOS:
image

Copy link
Member

@BeckerWdf BeckerWdf left a comment

Choose a reason for hiding this comment

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

does not work on macOS

@mvm-sap
Copy link
Contributor Author

mvm-sap commented Jan 3, 2025

does not work on macOS

I'm not sure why it isn't working. Shall I revert to a previous commit that made changes in all three platforms and worked fine? I don't have a MAC to test or debug, hence the problem.

@BeckerWdf
Copy link
Member

It's only the last change moving it to a common file that did not work. Your initial commit worked.

@mvm-sap
Copy link
Contributor Author

mvm-sap commented Jan 3, 2025

It's only the last change moving it to a common file that did not work. Your initial commit worked.

Shall I revert it back to the changes done in initial commit ?

@BeckerWdf
Copy link
Member

It's only the last change moving it to a common file that did not work. Your initial commit worked.

Shall I revert it back to the changes done in initial commit ?

yes

Selected sub-tabs in editor and views were missing blue highlight with
the dark theme changes. This has been fixed with this change.
@mvm-sap mvm-sap force-pushed the Dark_Theme_Sub_Tab_Highlight_Issue_Fix branch from f18b3fa to 496e376 Compare January 3, 2025 10:37
@mvm-sap
Copy link
Contributor Author

mvm-sap commented Jan 3, 2025

It's only the last change moving it to a common file that did not work. Your initial commit worked.

Shall I revert it back to the changes done in initial commit ?

yes

Could you please test again and confirm if its working?

@BeckerWdf
Copy link
Member

Works fine on macOS

@BeckerWdf BeckerWdf merged commit 6db4ad4 into eclipse-platform:master Jan 3, 2025
17 checks passed
@mvm-sap mvm-sap deleted the Dark_Theme_Sub_Tab_Highlight_Issue_Fix branch January 17, 2025 06:39
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.

2 participants