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	524825a
🔍 Latest deploy log	https://app.netlify.com/sites/expressjscom-preview/deploys/66e0f0a48b1c800008d0171f
😎 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?

[bjohansebas]

First PR (Refactor Menu):

• Remove dropit
• Remove functions related to dropit
• Refactor the menu according to the new structure
• Move menu styles to a new file
• Add new menu functionality in a new file

Second PR (Remove Unused Functions):

• Remove ismobil.js (
• Remove logic for page editing (no longer used since the footer was updated)
• Remove logic for window.location.pathname === '/3x/api.html' (no longer used since almost all URLs now go to /en/*)
• Remove function related to #tags-side-menu

Third PR (Adapt get elements):

• Update scroll top logic (@chrisdel101, should we keep the scroll logic to add a style when not at the top of the page?)
• Adapt element retrieval and events
• Add overlay logic to the menu
• Adapt code highlighting

Fourth PR (Adapt Functions):

• Update logic for the i18n banner
• Update logic for the closer title (used in the API)
• Adapt cookies logic to ES6

Fifth PR (Remove jQuery Dependency):

• Remove link to jQuery

Note: While writing the proposal, I realized that more PRs are needed to make the review easier.

@chrisdel101, do you agree with the proposal?

[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]

@chrisdel101, you can review now. I fixed some bugs I found in Chrome, so everything should work fine now.

[chrisdel101]

I wasn't a super reviewer on the header files since those are tricky to review. If there's any typos there hopefully we get them over time.
I left a bunch of comments to fix, and some suggestions too.
Great job with this. The navbar is finally a flex box! And the French nav is the correct way around.