Skip to content

feat(CodeEditor): use custom PatternFly monaco theme #11575

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

Closed
wants to merge 1 commit into from

Conversation

logonoff
Copy link
Member

@logonoff logonoff commented Feb 27, 2025

What: Closes #11506

Note there should be some additional CSS in PatternFly to make this look better (so that the margin around the CodeEditor matches the monaco background color), but I'm not sure of the best way to implement it:

.pf-v6-theme-dark {
  .pf-v6-c-code-editor:not(.pf-m-read-only) {
      --pf-v6-c-code-editor__main--BackgroundColor: var(--pf-t--color--gray--90);
      --pf-v6-c-code-editor__tab--BackgroundColor: var(--pf-t--color--gray--90);
  }
}

I tried looking into using pf-t--global--background--color--primary--default, but it doesn't provide enough contrast in dark mode. Meanwhile, pf-t--global--background--color--secondary--default doesn't provide enough contrast in light mode.

@patternfly-build
Copy link
Contributor

patternfly-build commented Feb 27, 2025

@logonoff logonoff force-pushed the codeeditor3 branch 3 times, most recently from b301164 to c875446 Compare February 27, 2025 20:31
@logonoff logonoff force-pushed the codeeditor3 branch 4 times, most recently from 8927aef to 701fdce Compare March 11, 2025 21:17
@logonoff logonoff changed the title refactor(CodeEditor): rewrite to become a functional component feat(CodeEditor): use custom PatternFly monaco theme Mar 11, 2025
@thatblindgeye thatblindgeye requested review from andrew-ronaldson, mcoker, a team, mfrances17 and kmcfaul and removed request for a team March 31, 2025 13:11
@nicolethoen nicolethoen requested a review from lboehling April 3, 2025 18:52
@thatblindgeye thatblindgeye requested review from a team and rebeccaalpert and removed request for mfrances17 and a team April 9, 2025 17:49
Copy link
Collaborator

@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

This is good. I tried to do a subtle rounding of the corners to match the container but it crops off too much so we can leave that for now

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Just needs a rebase

@logonoff
Copy link
Member Author

what was the verdict on adding the additional CSS so that the editor background colour matches the margin?

@thatblindgeye
Copy link
Contributor

@logonoff are you referring to the margin color shown below, where the background color surrounding the code text/of the "JAVASCRIPT" tab is lighter than the background color of the element containing the code "Some exam"?

image

@logonoff
Copy link
Member Author

@logonoff are you referring to the margin color shown below, where the background color surrounding the code text/of the "JAVASCRIPT" tab is lighter than the background color of the element containing the code "Some exam"?

image

yeah. in OCP I added some CSS so that these background colours match:

// match the token in theme.ts. it already matches for light theme
.pf-v6-theme-dark {
  .co-code-editor:not(.pf-m-read-only) {
      --pf-v6-c-code-editor__main--BackgroundColor: var(--pf-t--color--gray--90);
      --pf-v6-c-code-editor__tab--BackgroundColor: var(--pf-t--color--gray--90);
  }
}

the colours match in OCP

do you think it'd be possible (or even desirable) to include this in the base PatternFly styles? the issue is that there's no global semantic token that exists which has gray--90 in dark mode but white in light mode

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

I noticed some syntax color changes in light theme that didn't seem right, but looks like there might be a bug? Not sure if it's a bug in the code editor or just a workspace issue.

When you first load the page, the syntax highlighting looks money. But if you toggle the code editor dark theme on and off, syntax highlighting changes to colors that are not accessible.

Apr-14-2025.13-41-54.mov

For simple var declarations like the screenshot below

  • let is #21134d (a very dark blue)
  • name is black
  • "John" is #003366 (another dark blue)
Screenshot 2025-04-14 at 1 45 17 PM

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

🚀

@dlabaj
Copy link
Contributor

dlabaj commented Apr 21, 2025

closing request. Currently it doesn't support light mod a11y correctly, and there needs to be a more indepth conversation on how to implement this.

@dlabaj dlabaj closed this Apr 21, 2025
@logonoff
Copy link
Member Author

logonoff commented Apr 22, 2025

closing request. Currently it doesn't support light mod a11y correctly, and there needs to be a more indepth conversation on how to implement this.

hey! which a11y issues did you spot with the light mode? these need to be fixed as OCP uses this exact monaco theme

@mcoker
Copy link
Contributor

mcoker commented Apr 23, 2025

@logonoff AFAIK the only issue was the syntax highlighting in light theme. I included a small snippet at the bottom of this comment - #11575 (review). It appears like we lost most of the light theme syntax highlighting, but technically it looks like the colors just changed and don't have enough contrast to really be distinguishable from one another.

@logonoff
Copy link
Member Author

ah:

image

when choosing the colours I decided to lean more towards having 7:1 levels to contrast between each token and the background colour. however I guess this made the contrast between each colour not great..

@mcoker
Copy link
Contributor

mcoker commented Apr 23, 2025

@logonoff we maaayyy be able to get design feedback, but if I were taking a stab at it, I might just look at the default syntax highlighting that comes from Monaco, then pick the closest to those colors using our tokens. However, instead of using color palette tokens, I might use these "nonstatus" tokens we have for various colors - https://github.com/patternfly/patternfly/blob/a0dcdd6176f0a05d3dfc4ec198c1275c01b2ca61/src/patternfly/base/tokens/tokens-default.scss#L389-L415. The --default is usually the "default" (duh! haha) version of that color and --hover/clicked is a more pronounced version of the color (usually darker in light theme and lighter in dark theme).

IMO you could take a stab at it that way (or if you had another idea), and we can ask design to review, or I'm happy to hit up design first and see if they have suggestions - just lemme know!

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

Successfully merging this pull request may close these issues.

Spike: Investigate theming in Monaco editor
7 participants