Skip to content

Pr/48 dark mode #69

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

Closed
wants to merge 2 commits into from
Closed

Pr/48 dark mode #69

wants to merge 2 commits into from

Conversation

Ownezx
Copy link

@Ownezx Ownezx commented Feb 18, 2022

Addressing issue #48.

First commit is just after pulling, I had errors when I pulled up the tree renderer as it could not find the images in the project.
Second is adressing dark mode.

All unit test still run as I have only done cosmetics.
I also removed the texture rendering to avoid recreating a texture for each boxes every frame. This has the disadvantage of loosing the box borders.

Fixed an error where the asset path was not pointing to the right directory after forking the repo.
Implemented the possibility to easily choose the box style.
Made it so we can potentially change the box style later, and got rid of the borders to avoid calculating textures on the fly.
@ashblue
Copy link
Owner

ashblue commented Feb 19, 2022

@Ownezx super duper thank you for this. You caught me in the middle of a move across the country. I'll definitely take a detailed look at this. Super duper sorry for the delay in getting you a proper pull request review to move this through. Writing down a reminder in my tasks to get to this as soon as I'm settled in. Promise I will get to this, much excite.

Copy link
Owner

@ashblue ashblue left a comment

Choose a reason for hiding this comment

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

This looks great. Some minor changes I can knock out if you don't have time. I still need to do a full manual regression check on the GUI display, but that's the only outstanding thing here.

PS Sorry this took so long for a review. I've been in moving limbo the past 6 months. Had a very "Murphy's Law" kinda move


public ColorFader MainIconFader { get; }

public NodeFaders() {
Copy link
Owner

Choose a reason for hiding this comment

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

Digging this, makes sense.

_box.Height - _box.PaddingY);

if (_node.Task.HasBeenActive) {
GUI.backgroundColor = _faders.BackgroundFader.CurrentColor;
GUI.Box(rect, GUIContent.none, Styles.BoxActive.Style);

if (EditorGUIUtility.isProSkin) {
Copy link
Owner

Choose a reason for hiding this comment

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

Move if block to a static DisplaySettings.GetBoxActiveStyle() method that can support caching

}

else {
if (EditorGUIUtility.isProSkin) {
Copy link
Owner

Choose a reason for hiding this comment

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

Move if block to a static DisplaySettings.GetBoxInactiveStyle() method that can support caching

@Ownezx
Copy link
Author

Ownezx commented May 18, 2022

I have added that on my todo-List. I haven't touched Unity in a few months though (It's my turn to be busy), so if you want the changes to implemented timely, don't wait on me.

Take care.

@ashblue
Copy link
Owner

ashblue commented May 20, 2022

No worries, think I can tackle the minor touch-ups no problem. I totally get how schedules can fluctuate

@ashblue
Copy link
Owner

ashblue commented Jan 24, 2024

Sorry this is still in limbo. Been overloaded with game project(s) I'm on 😅

@ashblue
Copy link
Owner

ashblue commented Apr 22, 2024

There seems to be an issue with docked and undocked editor GUIs, looking into it. Looks like a bug on Unity's end potentially :(

@ashblue
Copy link
Owner

ashblue commented Nov 9, 2024

Closing in favor of #97

@ashblue ashblue closed this Nov 9, 2024
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.

2 participants