Skip to content

Conversation

@kathy239
Copy link
Contributor

Reference : Syntax_Highlighting.
The test result:
Test

Also, I suggest that we should sort the STYLE_MAP dictionary in alphabet order. I could quickly do it if everyone thinks it's a good idea.

Thanks xD

Copy link
Collaborator

@Anteru Anteru left a comment

Choose a reason for hiding this comment

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

I agree, that dictionary should be sorted, but I'll do it afterwards to not pollute this PR with unrelated changes.

BLUE_COMMENT = "#3F5FBF"
GREEN_COMMENT = "#3F7F5F"
BLACK = "#000000"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also please set:

    line_number_color
    line_number_background_color
    line_number_special_color
    line_number_special_background_color
    highlight_color

In addition to background_color, so you're not getting defaults that may or may not work for this? I suspect there's a particular highlight color you want to use for Eclipse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember Eclipse Light style use white as the background colour. Then, the default colour "#f8f8f8" should be alright. I thought white should be "#ffffff" as that is the result I got from Google.
Do you need me to just change the background_color to default? I think that default colour looks better.
I have already added the line number related stuff.
test

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding the other colors!

It's your style -- all I'm saying is the default colors might change one day, and hence I'd prefer if new styles define all colors we support to ensure they continue to work if we ever decide that the default should be bright pink. We haven't been paying much attention to this previously and it's yet another item on my todo list to automatically check this :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get it now. Thanks for your reply!
I think it's a good idea to keep my style not having too much adherent with the default settings.

I have already added the missing copyright header to the file. Now I am working on the contrast issue that @jeanas mentioned
Seems like my style failed a test due to not having enough contrast. Does that mean I need more "black&white" stuff to make my style more readable? Seems like whatever I try to edit the style and run the test_contrasts.py, I would only get the following error.

"AssertionError: contrast degradation for style 'eclipse'
The following rules have a contrast lower than the required 4.5:

  • 3.80 Token.Error"

Copy link
Contributor

Choose a reason for hiding this comment

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

Contrast is the minimum of contrasts for all tokens. In this case, you have to change the color of Error tokens to have a higher contrast with the white background (i.e., choose a darker color).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's no problem to have failing tests on a PR — to the contrary, we can help you figure out how to fix them and/or push commits ourselves to fix them.
Thanks for your help && reply
Hi, I changed the error to even darker colour like matte black. Seems like that number increased from 3.8 to 4.0! I probably need to tweak a few more colours to make my style a bit more readable.

@jeanas
Copy link
Contributor

jeanas commented May 24, 2023

There are CI failures.

  • Please add a copyright header to the file and run tox -e check to verify it's correct.
  • The style doesn't have a high enough contrast.

@kathy239
Copy link
Contributor Author

I just want to close it for now to keep the code clean. I am a newbie who is not quite familiar with the workflow. I think I need to pass all tests on my forked repo first before massing up with a pull request.

Thanks.

@kathy239 kathy239 closed this May 25, 2023
@jeanas
Copy link
Contributor

jeanas commented May 25, 2023

It's no problem to have failing tests on a PR — to the contrary, we can help you figure out how to fix them and/or push commits ourselves to fix them.

@kathy239 kathy239 reopened this May 25, 2023
@kathy239 kathy239 closed this Jun 8, 2023
@Anteru Anteru added the A-theming area: changes to themes label Aug 5, 2023
@Anteru Anteru added this to the 2.16.0 milestone Aug 5, 2023
@numist numist mentioned this pull request Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-theming area: changes to themes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants