-
Notifications
You must be signed in to change notification settings - Fork 36
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
add support for dark mode #259
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The preview and work looks amazing! I noticed that there is a cascading css rule for $black variable in same file
_sass/_variables.scss
Outdated
$white: #fff; | ||
$grey: #222; | ||
$grey2: #444; | ||
$grey3: #f6f6f6; | ||
$black: rgb(2, 14, 15); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this black will override the $black
definition on line 2. One of them should be removed, I'm happy to keep line 7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, i'd rather go with line 2 personally. not much difference either way
@@ -50,6 +37,16 @@ permalink: /nyc/ | |||
|
|||
<hr /> | |||
|
|||
<div class="flex justify-between ai-center pad"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ai-center
is such an intriguing name!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
atomic class names are just descriptive. sometimes i see this as items-center, but that's all it is. it aligns items along the vertical center
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd rather have tailwind just auto-generate all these and never think about css again, personally. for a future version maybe
addresses #250
not exactly the best, but hey, it works
for the svg icons, the reason i embedded them directly so i could take advantage of css classes to change the fill based on theme