feat - adjust in layout and theme - v2

[carlosstenzel]

Adjust in layout and theme

Before

https://github.com/expressjs/expressjs.com/assets/3890516/e123d4a2-294e-4998-9099-97c09bc592f4

After

https://github.com/expressjs/expressjs.com/assets/3890516/c600615f-05f7-46bb-a7fc-3a5d57d038eb

[netlify[bot]]

✅ Deploy Preview for expressjscom-preview ready!

Name	Link
🔨 Latest commit	3a3307f9f340e410aad4c829f307a423971b2756
🔍 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.

[chrisdel101]

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]

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]

@chrisdel101 I moved in another PR, this point about change icon in toggle theme https://github.com/expressjs/expressjs.com/pull/1532

[chrisdel101]

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