feat - adjust in layout and theme - v2 #1528

carlosstenzel · 2024-06-20T12:19:36Z

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

netlify · 2024-06-20T12:19:51Z

✅ Deploy Preview for expressjscom-preview ready!

Name	Link
🔨 Latest commit	`3a3307f`
🔍 Latest deploy log	https://app.netlify.com/sites/expressjscom-preview/deploys/667420323b6b4b000873b5fc
😎 Deploy Preview	https://deploy-preview-1528--expressjscom-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

…ut-and-theme-improvements-v2

chrisdel101 · 2024-06-23T19:02:20Z

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 importants for sure.

Random other things I'd suggest

I miss the border around the nav bar dropdowns. You've removed this.
On mobile view there is some strange x-scroll occurring on the API pages. This is not happening in production. Could just be my setup, but please check that out.
On a random page the text content stretches all the way to the edge of the page. We can't have this. I made comment about text block width conventions, or you can fall back to the current settings since they work okay.

carlosstenzel · 2024-06-24T13:07:34Z

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 importants for sure.

Random other things I'd suggest
* I miss the border around the nav bar dropdowns. You've removed this.

* On mobile view there is some strange x-scroll occurring on the API pages. This is not happening in production. Could just be my setup, but please check that out.

* On a random page the text content stretches all the way to the edge of the page. We can't have this. I made comment about text block width conventions, or you can fall back to the current settings since they work okay.

Thank you for the review, we can divide and treat each point separately.

I couldn't see your review comments, do I need permission?

carlosstenzel · 2024-06-24T13:21:43Z

@chrisdel101
I moved in another PR, this point about change icon in toggle theme
#1532

chrisdel101 · 2024-06-23T17:55:08Z

_includes/header/header-en.html

+      <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>


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.

I didn't find any other icon that could replace it :(

https://fontawesome.com/v4/icons/

I added another icon for the sun
#1532

with fa icons

new icon

chrisdel101 · 2024-06-23T17:57:52Z

_includes/header/header-en.html

@@ -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 ...">


I think the search bar should remain on the left side of the nav bar since moving it is a drastic-ish change. Also there's some overlap issues. Also happens on the Express Logo icon side too

chrisdel101 · 2024-06-23T18:07:19Z

css/dark-theme.css

 html.dark-mode > body {
-  background: var(--main_dark_bg);
+  background: #000;


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.

chrisdel101 · 2024-06-23T18:09:27Z

css/dark-theme.css

  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;


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.

chrisdel101 · 2024-06-23T18:14:06Z

css/style.css

+}
+
+.container-api {
+  width: 100%;


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

chrisdel101 · 2024-06-23T18:14:13Z

css/style.css

+.input-search {
+  border-radius: 8px;
+  border: 1px solid #ddd;
+  padding-left: 8px !important;


Never use important. I've been skunked a few times on this project by other ppl doing this.

chrisdel101 · 2024-06-23T18:14:46Z

css/style.css

+
+.input-search:focus-visible,
+.input-search:focus {
+  border-color: #333 !important;


Again, find a different way to do this. No important.

chrisdel101 · 2024-06-23T18:25:09Z

css/dark-theme.css

 html.dark-mode header {
  background-color: var(--second_dark_bg);
  color: var(--dark_text);
+  border-bottom: 1px solid #626262;


just personal preference but I like the white border of the current styles more. I just think it pops more.

chrisdel101 · 2024-06-23T18:27:19Z

css/dark-theme.css

 }
 /* 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;


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

chrisdel101 · 2024-06-23T18:31:11Z

css/style.css

-  font: 4.5em "Helvetica Neue", "Open Sans", sans-serif;
-  font-weight: 100;
+  font: 5em "Helvetica Neue", "Open Sans", sans-serif;
+  font-weight: 200;


This is too thick (I think). The current way it matches the Express icon more closely as well.

I like current way better.

chrisdel101 · 2024-06-24T14:05:17Z

I think I needed to submit the review. Hopefully you can see them now.

feat - adjust in layout and theme - v2

51ddb48

Merge remote-tracking branch 'origin/gh-pages' into feat/general-layo…

3a3307f

…ut-and-theme-improvements-v2

carlosstenzel mentioned this pull request Jun 20, 2024

feat - adjust in layout and theme #1522

Closed

chrisdel101 suggested changes Jun 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat - adjust in layout and theme - v2 #1528

feat - adjust in layout and theme - v2 #1528

carlosstenzel commented Jun 20, 2024

netlify bot commented Jun 20, 2024 •

edited

Loading

chrisdel101 commented Jun 23, 2024

carlosstenzel commented Jun 24, 2024

carlosstenzel commented Jun 24, 2024

chrisdel101 Jun 23, 2024

carlosstenzel Jun 24, 2024

carlosstenzel Jun 24, 2024

carlosstenzel Jun 24, 2024

chrisdel101 Jun 23, 2024

chrisdel101 Jun 23, 2024

chrisdel101 Jun 23, 2024

chrisdel101 Jun 23, 2024

chrisdel101 Jun 23, 2024

chrisdel101 Jun 23, 2024

chrisdel101 Jun 23, 2024

chrisdel101 Jun 23, 2024

chrisdel101 Jun 23, 2024

chrisdel101 commented Jun 24, 2024

feat - adjust in layout and theme - v2 #1528

Are you sure you want to change the base?

feat - adjust in layout and theme - v2 #1528

Conversation

carlosstenzel commented Jun 20, 2024

netlify bot commented Jun 20, 2024 • edited Loading

✅ Deploy Preview for expressjscom-preview ready!

chrisdel101 commented Jun 23, 2024

carlosstenzel commented Jun 24, 2024

carlosstenzel commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdel101 commented Jun 24, 2024

netlify bot commented Jun 20, 2024 •

edited

Loading