-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat - adjust in layout and theme - v2 #1528
base: gh-pages
Are you sure you want to change the base?
feat - adjust in layout and theme - v2 #1528
Conversation
✅ Deploy Preview for expressjscom-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…ut-and-theme-improvements-v2
Apologies for if anything appears overly critical, but there are alot of changes here so it just more room for critique :) Overall there are some good ideas here, but maybe too many? I'd personally rather have had you just do a PR that was say "Reformat Nav Bar," since the nav is problematic, rather than trying to tackle the entire site in one PR (and which I'm honestly not seeing a huge need for). If you are open to that, maybe we could just focus on the nav parts to start with? And try the rest in another PR? Anyway, I did some reviewing of the whole thing, but due to the size I could not check every single thing. I'd strongly suggest we do another pass over this again later. I made some comments. I put "personally" or "I think" in many of the comments since I don't see the need for them, but @crandmck might have a different outlook. The others probably do require the changes. Get rid of all the Random other things I'd suggest
|
Thank you for the review, we can divide and treat each point separately. I couldn't see your review comments, do I need permission? |
@chrisdel101 |
<input id="q" class="input-search" placeholder="Search ..."> | ||
<div id="theme-icon-container" class="theme-toggle default-theme" title="toggle darkmode"> | ||
<i class="fa fa-moon-o fa-lg hidden-dark"></i> | ||
<i class="fa fa-sun-o fa-lg hidden-light"></i> |
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.
Personally I don't like this icon b/c it looks like the "settings" gear icon to me. It's not clear that it means "light mode" immediately.
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 didn't find any other icon that could replace it :(
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 added another icon for the sun
#1532
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.
@@ -153,8 +154,11 @@ | |||
</li> | |||
--> | |||
</ul> | |||
<div id="theme-icon-container" class="theme-toggle default-theme" title="toggle darkmode"> | |||
<i class="fa fa-moon-o fa-2x"></i> | |||
<input id="q" class="input-search" placeholder="Search ..."> |
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.
html.dark-mode > body { | ||
background: var(--main_dark_bg); | ||
background: #000; |
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.
-
Why undo the variable with a hard-coded value? If you want a one-off change I suggest making it variable anyway.
-
And why change the color here anyway? I don't see the need for this.
color: var(--dark_text); | ||
} | ||
/* navbar links/drop down menu links */ | ||
html.dark-mode #navbar ul#navmenu li a, | ||
html.dark-mode #navbar ul#navmenu li.dropit-trigger a { | ||
color: var(--dark_text); | ||
color: #b6b6b6; |
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.
- Again, I don't see any reason to hardcode this. You should make it a variable.
- I personally like the white color that is is now, but I could be sold on this color if others like it more.
} | ||
|
||
.container-api { | ||
width: 100%; |
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 is too wide. Since the page is largely text it's awkward to read from edge to edge.
- The width of the styles currently works better.
- Standard I've always used is like a piece of paper, or a tablet size
.input-search { | ||
border-radius: 8px; | ||
border: 1px solid #ddd; | ||
padding-left: 8px !important; |
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.
- Never use important. I've been skunked a few times on this project by other ppl doing this.
|
||
.input-search:focus-visible, | ||
.input-search:focus { | ||
border-color: #333 !important; |
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.
- Again, find a different way to do this. No important.
html.dark-mode header { | ||
background-color: var(--second_dark_bg); | ||
color: var(--dark_text); | ||
border-bottom: 1px solid #626262; |
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.
- just personal preference but I like the white border of the current styles more. I just think it pops more.
} | ||
/* drop down menu links */ | ||
html.dark-mode #navbar .menu ul.dropit-submenu a:hover { | ||
background: var(--dark_hover); | ||
} | ||
|
||
html.dark-mode #navbar a.active { | ||
color: var(--dark_text) !important; |
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.
- Don't use important. Please find another way to make this work. You can make it more specific like
html.dark-mode #navbar ul#navmenu li a, html.dark-mode #navbar ul#navmenu li.dropit-trigger a
type of thing
font: 4.5em "Helvetica Neue", "Open Sans", sans-serif; | ||
font-weight: 100; | ||
font: 5em "Helvetica Neue", "Open Sans", sans-serif; | ||
font-weight: 200; |
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 is too thick (I think). The current way it matches the Express icon more closely as well.
- I like current way better.
I think I needed to submit the review. Hopefully you can see them now. |
Adjust in layout and theme
Before
Gravacao.de.Tela.2024-05-26.as.10.11.30.mov
After
Gravacao.de.Tela.2024-05-26.as.10.12.11.mov