Skip to content

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

Merged
Siedlerchr merged 4 commits into
JabRef:mainfrom
Asrk03:fix-for-issue-9192
Jan 15, 2023
Merged

Changed the color of light-color-text and highlighted text in …#9546
Siedlerchr merged 4 commits into
JabRef:mainfrom
Asrk03:fix-for-issue-9192

Conversation

@Asrk03

@Asrk03 Asrk03 commented Jan 6, 2023

Copy link
Copy Markdown
Contributor

…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.

Comment thread CHANGELOG.md Outdated
@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
Copy Markdown
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

Asrk03 commented Jan 8, 2023

Copy link
Copy Markdown
Contributor Author

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

@Siedlerchr

Copy link
Copy Markdown
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

ThiloteE commented Jan 14, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown
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

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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

Asrk03 commented Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

I would be happy with 2 or 3

@calixtus

Copy link
Copy Markdown
Member

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

@Asrk03

Asrk03 commented Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

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

@Asrk03

Asrk03 commented Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

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

Siedlerchr commented Jan 14, 2023

Copy link
Copy Markdown
Member

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

So I would suppose we decide now on that version

@Asrk03

Asrk03 commented Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

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

@Siedlerchr

Copy link
Copy Markdown
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

claell commented Jan 21, 2023

Copy link
Copy Markdown
Contributor

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

claell commented Jan 21, 2023

Copy link
Copy Markdown
Contributor

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

grafik

@ThiloteE

Copy link
Copy Markdown
Member

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

@claell

claell commented Jan 21, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

claell commented Oct 14, 2023

Copy link
Copy Markdown
Contributor

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

@ThiloteE

Copy link
Copy Markdown
Member

Yes, she has deleted her account.

@koppor

koppor commented Oct 20, 2023

Copy link
Copy Markdown
Member

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