sharmaeklavya2 / packing-game

2D geometric bin-packing game in the browser
https://sharmaeklavya2.github.io/packing-game/
8 stars 4 forks source link

added dark mode #12

Closed nikldev0 closed 3 years ago

nikldev0 commented 4 years ago

Associated Issue: [UI] Add dark theme #9

Summary of Changes

sharmaeklavya2 commented 4 years ago

A few changes are needed before I can accept your pull request:

Thanks for taking out the time to contribute.

nikldev0 commented 4 years ago

Oops, didn't mean to create so many commits.

Thanks so much for taking the time to give such detailed and educational feedback. I really appreciate it as a newbie :)

I've actioned all of your great feedback. The only thing is I didn't change the colors within the #hover-rect.success and #hover-rect.failure selectors as I felt they looked ok in dark mode. However, if you feel this isn't the best design choice let me know.

I've made the stats bar color a slightly different dark shade than the body and used CSS variables for all the colors I applied, let me know how this looks to you.

Thanks again for allowing me to contribute to your project.

sharmaeklavya2 commented 4 years ago

It looks better now. I agree with you on #hover-rect. There are still a few changes needed:

It looks like you didn't get the true spirit behind using custom CSS properties. You see, currently there is some duplication. As an example, outside the dark-mode media query, we have

#inventory, .bin {
    background-image: linear-gradient(to right, rgba(0,0,0,0.1) 1px, transparent 1px),
        linear-gradient(to bottom, rgba(0,0,0,0.1) 1px, transparent 1px);
}

and inside the dark-mode media query, we have

@media (prefers-color-scheme: dark) {
    #inventory, .bin {
        background-image: linear-gradient(to right, rgba(255,255,255,0.15) 1px, transparent 1px),
        linear-gradient(to bottom, rgba(255,255,255,0.15) 1px, transparent 1px);
    }
}

Instead, this rule should be only at one place, and only the part that is different for them, that is the color, should be specified using media queries. This is how it should be:

body {--grid-color: rgba(0,0,0,0.1);}
@media (prefers-color-scheme: dark) {
    body {--grid-color: rgba(255,255,255,0.15);}
}
#inventory, .bin {
    background-image: linear-gradient(to right, var(--grid-color) 1px, transparent 1px),
        linear-gradient(to bottom, var(--grid-color) 1px, transparent 1px);
}

Here --grid-color is just an example identifier. You can use other variable names if you like, but the names should make sense, of course. As an example, you'll see many colors of the form rgba(0, 0, 0, x) in style.css. I call them 'tints'. You can maybe define a few tint variables (0, 0, 0 for light mode and 255, 255, 255 for dark mode) and use them in place of the currently hard-coded rgba(0, 0, 0, x) values.

This approach of specifying colors separately from rules prevents us from essentially having 2 copies of each rule to maintain. You'll also be less likely to miss adding rules for dark mode. For example, you forgot to add box-shadow to .bin and .inventory. Depending of the grid size, sometimes the grid lines don't make good enough borders for the bin and inventory. So a border (or alternatively box-shadow) has to be explicitly added for them.

If you add a variable for every color, this can lead to a lot of different variables. Feel free to reduce the number of distinct colors in the light mode, as long as it looks aesthetically okay. For example, you can change rgba(0, 0, 0, 0.25) to rgba(0, 0, 0, 0.2) or rgba(0, 0, 0, 0.3) if it continues to look okay with this change.

Also, I want the status bar to be translucent, because when more bins are added, the status bar covers the arena.

You still have trailing whitespace. I suggest you configure your text editor to remove them or warn you of their presence. Also, before committing, you can run git diff --check and git will warn you about trailing whitespace.