new design and logic for the menu #1590

[bjohansebas]

The new menu logic is designed, and the new styles are added to work correctly.

Changes

• Remove dropit and its styles.
• The responsive design is fixed(Nav Bar styling - Spacing on Logo and Wrap are broken #1589)
• New styles

Preview

desktop

mobile

[netlify]

✅ Deploy Preview for expressjscom-preview ready!

Name	Link
🔨 Latest commit	dff6551
🔍 Latest deploy log	https://app.netlify.com/sites/expressjscom-preview/deploys/67202ab0f038160008b663a6
😎 Deploy Preview	https://deploy-preview-1568--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]

I'm personally somewhat ambivalent about all the JS refactors (also #1562, #1569). Is it refactoring for refactoring's sake?
Yes it's older tech, but IMO it still works fine for the use cases on a simple site.
Guess it's really up to @crandmck

[bjohansebas]

Yes it's older tech, but IMO it still works fine for the use cases on a simple site.

I agree with that, although on the other hand, wouldn't it be better to remove these dependencies that are only adding more load to the website? It might also be worth evaluating if these changes could optimize performance and maintenance in the long run.

[crandmck]

Sorry for the delay in responding, I was on vacation for the last two weeks :-)

I'm ambivalent about this. Since @bjohansebas has already done the work, I don't know that there's a good reason NOT to accept this PR--ASSUMING that it doesn't introduce any new bugs or glitches. Since I'm just getting back and catching up after vacay, I haven't had time to look more closely at this to ensure that's the case. @chrisdel101 have you?

We certainly appreciate the work you put in @bjohansebas but in the future it would be great to open an issue to discuss BEFORE doing the work for a PR. If you already did this and I just missed it, apologies, as I said I've been out for a couple weeks. I'm not saying that we won't accept a PR without prior discussion in an issue, but that's the preferred approach :-)

[bjohansebas]

@crandmck Sorry for rushing, collaborating on an open-source project is new to me. On the other hand, as you mentioned in #1562, it might be better to focus on more important things; whatever is best for the project.

[chrisdel101]

I agree with you @crandmck 100% about per-discussing PRs, especially ones like this that are ethos shifting, whereas maybe small bug fixes could slip by without?

This Improvements like this do have a legit place in web development. My thoughts are:

• It doesn't seem worth the effort to have redone this for this project. It's not like this is an application where benchmarks are essential.
• Only 1 file using JQ (I think) App.js, plus dropit.
• Pro: JQuery is coming via CDN so we're not using tons of space storing it...Con: although nowadays people are starting to hate on CDNs. Con: And the version 2.1.1 CDN could go away, sometime and then we'd need to do maintenance like this anyway.

So, since you did all this work already it seems like a waste to throw it out.

The main problems are:

• There 45 files in the PR (which I've glanced at briefly and saw things I had questions about already). Yes the changes might be very similar for each file, but this is alot at once to review.
• Changing ALL the JS at once means alot of testing.
• We need to be 100% sure the new JS works in all places before we merge this. It seems like this all might be pretty simple JS, but would take time just to double check every page since these are potentially breaking changes.

SO...

1. I'd be willing to review some of this stuff but I can't manage it across multiple PRs like it is. And, a single giant PR is not my preference either.
2. My thought is that you might have to redo these :( (not the work, just the arrangement)
3. If it's possible could you make us a simple listing (like a proposal) of the content you want to include across the PRs? Then implelement the first, like just redo drop-it in one PR, merge, then move to the next. This way they are not relying on each other.
4. If the 3 PRs already satisfy this, then having a proposal type-of -thing describing their order would help us see what is happening across all three.
5. Your "proposal" could just be an ordered list added to this thread.

This response got WAY too long and rambling. Did this make sense?

[chrisdel101]

Yes this looks good to me overall. Starting w/ the menu as you said is a good plan.
I think we can go ahead with PR # 1 as a separate PR, if you okay with that, at which point we'd prob just close this PR. But it'll be nice to have for this list and discussion.

Some things to consider:

• please try your best not to commit any prettifying changes (not saying you did but there are tons of changes in the header files and I can't tell if they are legit, or just prettifying). It's way easier to read and see the code changes without huge blocks of green and red.
• I like menu.css having a sep file too! But then I can't easily tell what has been changed. Not sure what the answer to this is...but just saying.
  • Maybe making that separate file a single change in one of the future PRs? Unless this is too gross.
• I like the CSS/markup menu fixes since it seems to fix things that were broken, at least on FF, and this is why I'd like to see clearly exactly what's been changed.

Is this okay @crandmck?

[crandmck]

Yes, this is a good approach. Thanks for taking the lead on this @chrisdel101. And, of course, thanks @bjohansebas for all the refactoring effort.

To help coordinate and simplify reviews, I would suggest opening an issue with the content of the above comment and then link that to all the PRs.

[bjohansebas]

I prefer not to impose a coding style. @chrisdel101, can you give it one last review so we can release it?

[chrisdel101]

I think maybe the express logo is too far in in large screens? If you agree, maybe this could stick the left corner more? I put the screen measurements in the corner so you can see where it's happening, or don't have a large screen.

I glanced at all the translations home pages and they all look good. For these, the logo is even further in, like https://deploy-preview-1568--expressjscom-preview.netlify.app/zh-tw/.

Unless this is what you are going for?

I got into testing this, and found a solution. I'll put it here, but you can do it your own way. I just spent the time playing around so thought I'd put it here. Or just ignore my code here. It's just an idea.
remove the header col gap

Add flex grow and margin-left


And then it seems to fix the translations too.

I do personally agree with removing the $ in the JS too just to keep the style consistent.
Everything else looks good that I checked. Once again, nice work. And thanks for making the first round of changes.

[bjohansebas]

@chrisdel101, thanks for the suggestion. I've opted for a different solution, let me know if you like it or not.

[chrisdel101]

I like it. Just maybe a few adjustments to the nav styles? For me there is too much space btw the logo and the search bar. What if you move the theme-icon-container out of the nav-menu? The only down side to this is that it requires changing every single header file
So like this

Then you don't need to change any css
You get this

Instead of this

If you dont want to change all the headers, you could also just flip to a space-around on the header. But I'm on only on a laptop screen rn so can't test a large screen to see if that fixes the original prob.
At the very least, I think there needs to be some space btw the "blog" nav item and dark/light button on the far right. And also maybe some space btw the search bar and "Home".

[bjohansebas]

I liked separating the theme-icon-container from the nav-menu more, this would help to add a language selector in the near future. I'll make the change later when I have some time

[chrisdel101]

Looks great! Sorry I missed this before.
So the only thing I notice is that the nav menu breakpoint kicks in too early which makes it awkward to use on a laptop. I def want to see the nav links still at the same size in the new format. Can you lower the breakpoint to kick in at like 899 (like it was before?)
This is the current way. I can see the nav menu which I def want at this size.

And this is changed way

I guess it's this rule but I can't seem to change it in fire fox so I'm not able to see what it looks like. Change this back?

Old setting (with nav hidden) - can u see the breakpoint to this value?

[bjohansebas]

@chrisdel101 Yeah, that's true, but the problem is that there are many items in the menu that, if it doesn't start from that point, it breaks the layout and this issue(#1589) would occur.

[bjohansebas]

I believe all the issues are already resolved, we can always make changes later.

I need the approval of a person with write permissions to merge it (@UlisesGascon, @expressjs/docs-captains).

[chrisdel101]

Yeah your right. Maybe we can hide the last few links or something, since I don't want to have to click the hamburger all the time.
But yeah, can deal with this later then.

LGTM.

[bjohansebas]

I've brought the changes made in #1674 and #1667 to avoid conflicts

[bjohansebas]

@chrisdel101 thanks for helping me get this onto the website