Skip to content

Dark Theme v2.0 #381

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

Merged
merged 15 commits into from
May 9, 2025
Merged

Conversation

aguero-tech
Copy link
Contributor

@vbakke & @wurstbrot thanks for the feedback on #374, since we are making major angular upgrades, i took off the matrix buttons, so this PR solely focuses on dark theme improvements.

  1. Moved light/dark toggle button to the side nav
  2. icon toggle colors match the theme.
  3. changed the background and headings of text/drawers (in dark mode)
  4. made the heatmap gridlines & text white to contrast with dark background (in dark mode).

Sorry i did not change the DSOMM in black on the top left. I think it looks pretty rad, and i didn't want to load another image lol.
image

@aguero-tech aguero-tech marked this pull request as ready for review March 25, 2025 21:31
@aguero-tech
Copy link
Contributor Author

image

@wurstbrot wurstbrot requested a review from 0x41head March 29, 2025 16:32
@vbakke
Copy link
Collaborator

vbakke commented Mar 30, 2025

@aguero-tech , I created a pull request to your pull request, where the circular heatmap is drawn in dark / light mode.

However, switching the light mode from the menu, currently does not update the setting of the circular component. Are you able to pass on the switch to the circular component?

@vbakke
Copy link
Collaborator

vbakke commented Mar 30, 2025

@aguero-tech , looks like adding the 'Switch to Dark Mode' is breaking the unittest check for navigation names being shown in the same order as options array.

@0x41head: I'm a bit unsure about the benefit of this test, and what sort of mistakes this test is supposed to uncover. Is it meaningful?

@aguero-tech
Copy link
Contributor Author

Thank you @vbakke . I apologize. im out for the weekend. Will touch base on Tuesday. I'll look into the menu unit test break as well and see if I can ameliorate it or find some enlightenment about the purpose of the test as well. I appreciate it.

@vbakke
Copy link
Collaborator

vbakke commented Mar 31, 2025

Not to worry @aguero-tech . I think I broke everything that cold be broken on my first PR. : )

Nor is this any rush. You're doing it in your spare time. Do it when time allows 🙂

@0x41head
Copy link
Collaborator

fixed the unit test @vbakke @aguero-tech

@vbakke
Copy link
Collaborator

vbakke commented Apr 13, 2025

How are you doing @aguero-tech?

Do you see any issues regarding the PR aguero-tech#11 ?
Or do you need any assistance with Angular and passing the theme to the CircularHeatmapComponent?

@aguero-tech
Copy link
Contributor Author

aguero-tech commented Apr 18, 2025

That was fun to work on. (learning is fun) @vbakke apologies for being absent. I actually got in a minor motorcycle accident and was out for a couple weeks with a messed up shoulder, luckily no fractures/broken bones. All good! Here's a clip for proof of concept. Light theme has white graph background, while the dark theme has light gray. I hope this is close to what you had in mind. Feel free to change the colors under custom-theme.scss (under .dark-theme line 152) the trigger is done via theme.service.ts under the app>service folder.

DSOMM.-.Opera.2025-04-17.20-42-35.mp4

@vbakke vbakke self-requested a review April 18, 2025 08:28
@vbakke
Copy link
Collaborator

vbakke commented Apr 18, 2025

Dude, no need to apologize. We do this on our spare time. And having an_accident_ is definitely a good reason to focus on yourself! 😨

I was just meaning to check in. I don't always pick up on messages from GitHub myself, so I was just going to check that you weren't thinking you were waiting for feedback 😊

I'll have a look at the end result. Probably over the Easter weekend or so. Take care, both on and off the road 🤞😃

Copy link
Collaborator

@vbakke vbakke left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, @aguero-tech. Please see my comments. Feel free to disagree to some of them. Just let me know why :)

…olors. Incorporate4d lots of vbakke's requests (cleaning up code). Fixed the persistence of theme after refresh.
@aguero-tech aguero-tech requested a review from vbakke April 23, 2025 15:47
@aguero-tech
Copy link
Contributor Author

aguero-tech commented Apr 23, 2025

@vbakke thank you very much for being thurough and working on this together. Been a good time so far. I'm done with the fixes. All yours! Let me know how i did!! selected ya for re-review.

Copy link
Collaborator

@vbakke vbakke left a comment

Choose a reason for hiding this comment

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

Much better! Thank you @aguero-tech! :)

I found a few more minor things.

  • In teams.component.css we get a very low contrast on the teams' names. I suggest we add:
    color: white; to the .team-list ul li { ... } (for both light and dark mode)

  • In styles.css (used in 'DSOMM User Day 2024' (not sure why it is still here)), the green table row need adjustments in the dark mode:

.dark-theme .userday table :is(td, th) {
    border-color: #e0e0e0;
}
.dark-theme .userday tr:nth-child(even) {
    background-color: #365d36;
}

… online 106). (Line-63; updating link colors to determine by browser visited state for dark theme. (Line-159; moving app-matrix to respective componenet.css and fix for visited. Fix on styles.css for dark theme on About Us.
… online 106). (Line-63; updating link colors to determine by browser visited state for dark theme. (Line-159; moving app-matrix to respective componenet.css and fix for visited. Fix on styles.css for dark theme on About Us.
@aguero-tech aguero-tech requested a review from vbakke April 30, 2025 18:56
@aguero-tech
Copy link
Contributor Author

@vbakke all very good recommendations! Thanks again!

Copy link
Collaborator

@vbakke vbakke left a comment

Choose a reason for hiding this comment

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

Think we are there now. Just two minor issues

@aguero-tech
Copy link
Contributor Author

Thanks @vbakke! We did it! lol first round on me.

vbakke
vbakke previously approved these changes May 5, 2025
Copy link
Collaborator

@vbakke vbakke left a comment

Choose a reason for hiding this comment

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

Excellent, @aguero-tech! Looking forward to that! :) 🍻

Looks good to me now @wurstbrot / @0x41head . :) 👍

.title-button,
h1, h2, h3, h4, h5, h6 {
color: map-get($custom-dark-theme, text);

Copy link
Collaborator

Choose a reason for hiding this comment

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

please format the CSS and remove whitespaces

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 have removed extra lines and spaces and have consolidated some ".body.dark-theme" code. Some lines i did not align just to keep it nested, but we can move it if desired.

background-color: #74b277;
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

please format the CSS and remove whitespaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

formatted.




@include mat.all-component-themes($DSOMM-dark-theme);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is here a space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spaces removed.

@wurstbrot
Copy link
Collaborator

@aguero-tech thank you. Great idea. Please run auto format with a tool over the files or fix the comments.
In the future, signed commits would be good.

@aguero-tech
Copy link
Contributor Author

I ran an auto formatting tool and didn't like what it changed, so i manually removed extra spaces and lines. Let me work on re-establishing my signature. Sorry I let it expire and forgot about it.

… fixed my GPG key signature for contribution - Manny
@aguero-tech
Copy link
Contributor Author

Thanks for lessons on code quality and styling @wurstbrot and @vbakke! I think this should be it. I finished my reformatting and got my GPG signed. Let me know if there is anything else! Thanks gentlemen!

@0x41head
Copy link
Collaborator

0x41head commented May 8, 2025

Can we change the text color for the acyclic graph as well ?
image

@aguero-tech
Copy link
Contributor Author

Oh thats a good find @0x41head! Not sure where that is but I'll hunt it down.

@0x41head
Copy link
Collaborator

0x41head commented May 8, 2025

@aguero-tech let me know if you have difficulties finding the root cause of the bug 👍

@aguero-tech
Copy link
Contributor Author

aguero-tech commented May 8, 2025

Thanks @0x41head!

image My color edit changed the parameter "activity" to unused. Line 143.
image.

But other than that. were ok.
image
image

… mode. Also subscribed the dependency graph text to theme.service to dynamically change text color w/o refresh. Basic Linting.
@0x41head
Copy link
Collaborator

0x41head commented May 8, 2025

LGTM! Thanks for the work on this PR @aguero-tech 👍

@wurstbrot wurstbrot merged commit 87d6312 into devsecopsmaturitymodel:master May 9, 2025
2 checks passed
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.

4 participants