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

Changed the color of light-color-text and highlighted text in … #9546

Merged
merged 4 commits into from
Jan 15, 2023

Conversation

Asrk03
Copy link
Contributor

@Asrk03 Asrk03 commented Jan 6, 2023

…entry merge dialogue

Fixes #9192

Before:

grafik
grafik

After:
Screenshot 2023-01-14 235730
Screenshot 2023-01-15 004947
Screenshot 2023-01-15 005014
Screenshot 2023-01-15 005032

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

CHANGELOG.md Outdated Show resolved Hide resolved
@Asrk03 Asrk03 changed the title Change the the color of light-color-text and highlighted text in … Changed the color of light-color-text and highlighted text in … Jan 6, 2023
@Siedlerchr
Copy link
Member

Thanks for your work, I am not that happy with the chosen colors.
I think maybe something similar to the search color? Maybe you can also check some contras colors https://coolors.co/contrast-checker/

Search highlight color:
grafik

@Asrk03
Copy link
Contributor Author

Asrk03 commented Jan 8, 2023

@Siedlerchr thank you for your response, will work on changes now.

@Siedlerchr
Copy link
Member

Thanks for the updated colors! Looks better now for me. Let's ask the others. @JabRef/developers

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 14, 2023
@ThiloteE
Copy link
Member

ThiloteE commented Jan 14, 2023

I think light yellow and light orange do not contrast with each other greatly, but oh well. Definitely better than before! I personally find it easier to read now. Thanks! I am in favour of merging this PR, unless somebody comes up with an even better solution.

With this PR:
image

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member

  • I like the change of text color to black

If we are interested in a discussion, I liked the dull red/sky blue for delete/add, but perhaps a higher contrast blue, maybe #006dff 😛

Anyway, I agree that this is a clear improvement

Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left a comment

Choose a reason for hiding this comment

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

Just for clarity, I am in favor of merging the PR and perhaps discuss further during the dev-call?

@ThiloteE
Copy link
Member

How about these three colours?

  • #ADD, #AADDDD, or rgb(170,221,221)
  • #FEC, #FFEECC, or rgb(255,238,204)
  • #F99, #FF9999, or rgb(255,153,153)

Source: https://en.wikipedia.org/wiki/File:Safe_Chart_Colors-F99-FEC-ADD.jpg

Was taken from the wikipedia entry about colour blindness.
https://en.wikipedia.org/wiki/Color_blindness

@Asrk03
Copy link
Contributor Author

Asrk03 commented Jan 14, 2023

Thank you for your suggestions!
Please let me know which one you like. I will make respective changes.
Personally I like 2nd better here.

Screenshot 2023-01-14 210904

Screenshot 2023-01-14 211223

Screenshot 2023-01-14 211301

Screenshot 2023-01-14 212338

@Siedlerchr
Copy link
Member

I would be happy with 2 or 3

@calixtus
Copy link
Member

How about the red from 4 with the orange from 3?

@Asrk03
Copy link
Contributor Author

Asrk03 commented Jan 14, 2023

@calixtus I think this combo is not very pleasing to eyes
Screenshot 2023-01-14 235730

@Asrk03
Copy link
Contributor Author

Asrk03 commented Jan 14, 2023

I think in this image the texts are discernable and the colors contrast well with each other too.
Should I finalize these colors?
image

@Siedlerchr
Copy link
Member

Siedlerchr commented Jan 14, 2023

@Asrk03
I agree, I would go for the last version here: #9546 (comment)

So I would suppose we decide now on that version

@Asrk03
Copy link
Contributor Author

Asrk03 commented Jan 14, 2023

I have made the changes please review them @Siedlerchr @k3KAW8Pnf7mkmdSMPHz27 @ThiloteE @calixtus

@Siedlerchr
Copy link
Member

Thanks again for your contributions and the updates!

@Siedlerchr Siedlerchr merged commit eeb5f25 into JabRef:main Jan 15, 2023
@Asrk03 Asrk03 deleted the fix-for-issue-9192 branch January 16, 2023 17:58
Siedlerchr added a commit that referenced this pull request Jan 16, 2023
…ialog

* upstream/main:
  Bump xmlunit-core from 2.9.0 to 2.9.1
  Bump mockito-core from 4.11.0 to 5.0.0
  Bump xmlunit-matchers from 2.9.0 to 2.9.1
  Bump junit-platform-launcher from 1.9.1 to 1.9.2
  Squashed 'buildres/csl/csl-styles/' changes from 43566f2..50eea55b2c
  New translations JabRef_en.properties (Portuguese, Brazilian) (#9559)
  Changed the color of light-color-text and highlighted text in … (#9546)
  New translations JabRef_en.properties (Portuguese, Brazilian) (#9557)
  chore: improve debug output in powershell starter script (#9550)
  Show development information (#9555)
  Observable Preferences T (NameDisplayPreferences, MainTablePreferences, ColumnPreferences) (#9535)
@claell
Copy link
Contributor

claell commented Jan 21, 2023

Sorry, I am late. Right now, it seems that the meaning of the colors are no longer there. At least in git, it is common to use red for deletions and green for additions (and neutral if nothing changed). I feel it is sensible to preserve that scheme, no? I can also open a new issue for that in case this is preferred.

@claell
Copy link
Contributor

claell commented Jan 21, 2023

From Visual Studio Code (https://code.visualstudio.com/docs/sourcecontrol/overview):

grafik

@ThiloteE
Copy link
Member

The red green combo is particular problematic for people with colour blindness.

@claell
Copy link
Contributor

claell commented Jan 21, 2023

Yes, I saw that afterward. The thing is that the colors are used here to show information. So it might be hard to keep track of the changes when they are not red or green, I think. But I haven't tested, probably one can get used to that. I need to test that before making further statements on that.

When I upload the screenshot from VS Code to a color blindness simulator, the shades can still be differentiated; although not as much as the ones right now.

@ThiloteE
Copy link
Member

@the-dino-girl, since you are very enthusiastic about JabRef themes, I wanted to make you aware of these issues here. Just so you know. Maybe you find some great looking combination that is both pleasing to the eyes, conservative and close enough to the red / green combination that is traditionally used by github and other programms to denote removal and addition of code and also does not exclude people with colour blindness.

@claell
Copy link
Contributor

claell commented Oct 14, 2023

@ThiloteE the user reference above isn't working. Has the user left GitHub or is there some spelling mistake?

@ThiloteE
Copy link
Member

Yes, she has deleted her account.

@koppor
Copy link
Member

koppor commented Oct 20, 2023

I can also open a new issue for that in case this is preferred.

@claell Yes, please do. Please try to collect the information provided here in a structured form. Especially MADR could be of help here. Feel free to open a pull request with the different options (and links of them). Then we can decide and then move forward to an implementation.

Reasoning: For a quick reader, it is very hard to follow the discussion here and to be able to decide something. Is it just about changing a color back to something or is it about a general UI design of JabRef? - Refs #7771.

@Asrk03 Asrk03 restored the fix-for-issue-9192 branch February 7, 2024 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change dark theme colour of highlighted text in Entry Merge Dialogue
7 participants