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

Make chart background color darker (dark theme) #382

Merged

Conversation

thefinaljob
Copy link
Contributor

@thefinaljob thefinaljob commented Jun 4, 2021

Changed the code to make the background color darker for the chart. It is now a shade of dark blue.

Contributes towards fixing #282

@thefinaljob thefinaljob force-pushed the background-color-dark branch from 1d54b4f to b64b53d Compare June 4, 2021 13:23
Copy link
Contributor

@MatthewKhouzam MatthewKhouzam left a comment

Choose a reason for hiding this comment

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

Hi thefinaljob,

thanks for the patch!

The commit message is kind of unclear. this changes the background color in light and dark theme
Light theme passes from https://duckduckgo.com/?q=color+picker+%23f4f7fb&ia=answer to https://duckduckgo.com/?q=color+picker+%23333f52&ia=answer

I find this not pretty. Maybe just change the dark one?

@MatthewKhouzam MatthewKhouzam requested a review from ebugden June 4, 2021 15:42
@MatthewKhouzam
Copy link
Contributor

Here's a quick helper for commit messages.

https://www.freecodecamp.org/news/writing-good-commit-messages-a-practical-guide/

I am not always the best citizen on that front though.

Could look like this

Capitalized, short (50 chars or less) summary

More detailed explanatory text, if necessary. Wrap it to about 72
characters or so. In some contexts, the first line is treated as the
subject of an email and the rest of the text as the body. The blank
line separating the summary from the body is critical (unless you omit
the body entirely); tools like rebase can get confused if you run the
two together.

Write your commit message in the imperative: "Fix bug" and not "Fixed bug"
or "Fixes bug." This convention matches up with commit messages generated
by commands like git merge and git revert.

Further paragraphs come after blank lines.

  • Bullet points are okay, too

  • Typically a hyphen or asterisk is used for the bullet, followed by a
    single space, with blank lines in between, but conventions vary here

  • Use a hanging indent

If you use an issue tracker, add a reference(s) to them at the bottom,
like so:

Resolves: #123

Copy link
Contributor

@ebugden ebugden left a comment

Choose a reason for hiding this comment

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

Requested changes 🌿

  • Include the issue that this contributes towards fixing (Make chart and filter field backgrounds a darker colour in dark theme #282) in the commit message and PR description
  • For light theme: Change colour back to the the original theme background. The contrast was already good and low in the light theme before this proposed patch.
  • For dark theme: Change colour to a neutral grey that is slightly lighter than the tab background (ex. #232323) (neutral grey: same values for red, green and blue). The tab background and the rest of the greys in the UI are often neutral. The chart background should be a shade of the tab background colour (same hue/saturation just different brightness)

If you want more details, you can check out the issue description #282. I just made some additions to clarify the requirements/motivation for the change.

The main goal of these changes is to decrease the contrast between the tab background and chart background. If you convert the colours to HSB you can see that the colour proposed in this PR has a higher difference in brightness with the tab background than the original grey (i.e. the colour proposed here increases the contrast rather than decreasing it).

Current background Currently proposed background
Difference in Brightness (HSB) between chart and tab: ~13 image Difference in Brightness (HSB) between chart and tab: ~20 (higher contrast with tab background than original grey) image
Chart background: #3f3f3f image Tab background: #1e1e1e image Chart background: #333f52 image Tab background: #1e1e1e image

Light theme should look like this:
image

@MatthewKhouzam
Copy link
Contributor

@ebugden Could you illustrate that with light theme too? my main issue is that it looks bad with light theme.

@MatthewKhouzam
Copy link
Contributor

image
Here's what it looks like in light theme.

@ebugden
Copy link
Contributor

ebugden commented Jun 8, 2021

@MatthewKhouzam I agree the current changes look really bad in light theme. I just adjusted my change requests to make this more clear. I'm aiming for something like this for light and dark themes:

Current Light Theme Proposed Adjusted Dark Theme
Difference in brightness (HSB) between chart and tab: ~2 image Difference in brightness (HSB) between chart and tab: ~2 image
Chart background: #f4f7fb image Tab background: #ffffff image Chart background: #232323 image Tab background: #1e1e1e image

Copy link
Contributor

@ebugden ebugden left a comment

Choose a reason for hiding this comment

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

Requested changes:

  • Update chartBackgroundColor in updateBackgroundTheme( ) as well. Right now when you switch to light theme and back to dark, it changes back to the light grey (see video below).
  • Include the issue that this contributes towards fixing (Make chart and filter field backgrounds a darker colour in dark theme #282) in the commit message and PR description. This PR doesn't make the filter field darker so the issue shouldn't be closed when this PR is merged.
  • Write commit title in imperative: "Make chart background color darker"
  • Add signoff to commit
  • Apply corrections by amending and force pushing the original commit instead of creating a new one. There should be just one commit in the PR. (Let me know if you want some help doing this!)

Thanks for updating the background colour! 🌼 I find this darker background to be less distracting.

Current background 🌈 PR Proposed background
image image

Currently the chart background was only updated in the constructor not in updateBackgroundTheme( ). So when you switch to light theme and back to dark theme the colour is still the light grey (see 0:37 in the video):

TraceCompassTutorialTraces.Theia-Trace.Example.Application.-.Google.Chrome.2021-07-21.15-25-12.mp4

Copy link
Contributor

@MatthewKhouzam MatthewKhouzam left a comment

Choose a reason for hiding this comment

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

please squash the two patches... we can't have both merged. ;)

thefinaljob added a commit to thefinaljob/theia-trace-extension that referenced this pull request Jul 23, 2021
…loud#382

Signed-off-by: Nikolai Peram <nikolai_peram@outlook.com>
@thefinaljob thefinaljob force-pushed the background-color-dark branch from 613ea53 to b64b53d Compare July 23, 2021 13:04
thefinaljob added a commit to thefinaljob/theia-trace-extension that referenced this pull request Jul 23, 2021
…loud#382

Signed-off-by: Nikolai Peram <nikolai_peram@outlook.com>
@thefinaljob thefinaljob force-pushed the background-color-dark branch from 7a2581f to b64b53d Compare July 23, 2021 13:13
thefinaljob added a commit to thefinaljob/theia-trace-extension that referenced this pull request Jul 23, 2021
…loud#382

Signed-off-by: Nikolai Peram <nikolai_peram@outlook.com>
thefinaljob added a commit to thefinaljob/theia-trace-extension that referenced this pull request Jul 23, 2021
…loud#382

Signed-off-by: Nikolai Peram <nikolai_peram@outlook.com>
@thefinaljob thefinaljob force-pushed the background-color-dark branch from 36105f9 to bc3cf84 Compare July 23, 2021 13:21
thefinaljob added a commit to thefinaljob/theia-trace-extension that referenced this pull request Jul 23, 2021
…loud#382

Signed-off-by: Nikolai Peram <nikolai_peram@outlook.com>
@thefinaljob thefinaljob force-pushed the background-color-dark branch from b08a938 to 7a6667c Compare July 23, 2021 13:43
thefinaljob added a commit to thefinaljob/theia-trace-extension that referenced this pull request Jul 23, 2021
…loud#382

Signed-off-by: Nikolai Peram <nikolai_peram@outlook.com>
@MatthewKhouzam
Copy link
Contributor

Hi... er... I think you passed from 2 patches to 0 patches. How is this PR empty. I'm confused.

thefinaljob added a commit to thefinaljob/theia-trace-extension that referenced this pull request Jul 23, 2021
…loud#382

Signed-off-by: Nikolai Peram <nikolai_peram@outlook.com>
thefinaljob added a commit to thefinaljob/theia-trace-extension that referenced this pull request Jul 23, 2021
…loud#382

Signed-off-by: Nikolai Peram <nikolai_peram@outlook.com>
@thefinaljob thefinaljob changed the title made background color darker make background color darker Jul 23, 2021
thefinaljob added a commit to thefinaljob/theia-trace-extension that referenced this pull request Jul 23, 2021
…loud#382

Signed-off-by: Nikolai Peram <nikolai_peram@outlook.com>
@thefinaljob thefinaljob reopened this Jul 23, 2021
MatthewKhouzam
MatthewKhouzam previously approved these changes Jul 23, 2021
Copy link
Contributor

@MatthewKhouzam MatthewKhouzam left a comment

Choose a reason for hiding this comment

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

WHooooooooooooo!

thefinaljob added a commit to thefinaljob/theia-trace-extension that referenced this pull request Jul 23, 2021
…loud#382

Signed-off-by: Nikolai Peram <nikolai_peram@outlook.com>
@thefinaljob thefinaljob force-pushed the background-color-dark branch from e538c72 to b6b7d46 Compare July 23, 2021 15:06
MatthewKhouzam
MatthewKhouzam previously approved these changes Jul 23, 2021
Copy link
Contributor

@MatthewKhouzam MatthewKhouzam left a comment

Choose a reason for hiding this comment

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

Wasn't there a light theme change too? I guess this is OK.

@ebugden ebugden changed the title make background color darker Make background color darker Jul 23, 2021
@ebugden
Copy link
Contributor

ebugden commented Jul 23, 2021

Wasn't there a light theme change too? I guess this is OK.

No the light theme was good already. The goal of this change was to have lower contrast between the tab background and chart background in the dark theme (similar to the low contrast already in the light theme) (See #282).

The darker backgrounds in some of the light theme screenshots are because of an update bug that I think was fixed recently by @muddana-satish #407.

Here's what these changes look like in dark and light theme:

TraceCompassTutorialTraces.Theia-Trace.Example.Application.-.Google.Chrome.2021-07-23.11-57-06.mp4

Copy link
Contributor

@ebugden ebugden left a comment

Choose a reason for hiding this comment

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

One last change!

@ebugden ebugden changed the title Make background color darker Make chart background color darker (dark theme) Jul 23, 2021
…loud#282

Signed-off-by: Nikolai Peram <nikolai_peram@outlook.com>
Copy link
Contributor

@ebugden ebugden left a comment

Choose a reason for hiding this comment

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

Looks good to me! 🌵 @thefinaljob We can merge after approval from a second committer. I just sent the additional review request so it can take a little while.

@ebugden ebugden requested review from bhufmann and a user July 29, 2021 14:46
Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@IbrahimFradj IbrahimFradj self-requested a review August 23, 2021 21:35
Copy link
Contributor

@IbrahimFradj IbrahimFradj left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you.

@bhufmann bhufmann merged commit aa86ded into eclipse-cdt-cloud:master Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants