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

more lovable text colors #203692

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Dec 10, 2024

Related to #199715
Closes #200160

Summary

The Kibana ES|QL + Discover teams reviewed the ES|QL editor text colors and found them to be dull relative to current theme settings. This PR makes a slight change to use base colors instead of text colors.

Note
Longer term - and this consideration was preexisting - we could explore adding a text-editor-specific set of tokens for use in Monaco editors. That said, these theme files are not React based, so this might be a challenge to handle via EUI tokens once the old javascript based theme tokens are deprecated.

Before and after

Editor in 8.X (Amsterdam theme)

Editor on main (Borealis theme)

Editor after this PR (Borealis theme)
Light mode

Dark mode

@ryankeairns ryankeairns requested a review from a team as a code owner December 10, 2024 21:18
@ryankeairns ryankeairns added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v9.0.0 labels Dec 10, 2024
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
kbnUiSharedDeps-srcJs 3.5MB 3.5MB -20.0B

@ryankeairns
Copy link
Contributor Author

Need to fix comments color in dark mode, too:

CleanShot 2024-12-10 at 16 36 12@2x

@stratoula should this be fixed separately so that it can land in 8.17.X or 8.18?

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Awesome Ryan, thank you so much <3

@stratoula
Copy link
Contributor

Need to fix comments color in dark mode, too:

CleanShot 2024-12-10 at 16 36 12@2x

@stratoula should this be fixed separately so that it can land in 8.17.X or 8.18?

Yes is def a separate issue. Have we identified which grey looks better in dark mode?

@ryankeairns ryankeairns merged commit 6f5d5cd into elastic:main Dec 11, 2024
17 checks passed
@ryankeairns
Copy link
Contributor Author

Need to fix comments color in dark mode, too:

@stratoula should this be fixed separately so that it can land in 8.17.X or 8.18?

Yes is def a separate issue. Have we identified which grey looks better in dark mode?

@stratoula I tried euiThemeVars.euiTextSubduedColor (change from euiTextDisabledColor) and that does the trick.
We could keep the Disabled color for light mode... or just use Subdued for both modes. Curious your personal preference when looking at this.

@stratoula
Copy link
Contributor

I will check it and let you know Ryan!

CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
Related to elastic#199715
Closes elastic#200160

## Summary

The Kibana ES|QL + Discover teams reviewed the ES|QL editor text colors
and found them to be dull relative to current theme settings. This PR
makes a slight change to use base colors instead of text colors.

**Note**
Longer term - and this consideration was preexisting - we could explore
adding a text-editor-specific set of tokens for use in Monaco editors.
That said, these theme files are not React based, so this might be a
challenge to handle via EUI tokens once the old javascript based theme
tokens are deprecated.

### Before and after

**Editor in 8.X (Amsterdam theme)**
<img width="420"
src="https://github.com/user-attachments/assets/23726e98-9277-4ef4-ae21-c843fc929aa3"
/>

**Editor on main (Borealis theme)**
<img width="400"
src="https://github.com/user-attachments/assets/2b973b8b-d292-46d2-a5d2-0a787f8c686a"
/>

**Editor after this PR (Borealis theme)**
_Light mode_
<img width="600"
src="https://github.com/user-attachments/assets/d34f2961-44b9-4f79-8690-dc44035f8146"
/>

_Dark mode_
<img width="420"
src="https://github.com/user-attachments/assets/e4ccbfb9-4dc1-4c41-ad95-6cc54a8de575"
/>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting EUI Visual Refresh release_note:skip Skip the PR/issue when compiling release notes v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ES|QL] Testing the new borealis theme in the editor
3 participants