Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link
  • Ideally put your CSS in a separate file to keep your markup clean.

    Ideally put your CSS in a separate file to keep your markup clean.

  • I put a function around your code so that the symbols can be properly minified.

    I put a function around your code so that the symbols can be properly minified.

  • I introduced a menuVisible variable which will be false if the menu is not showing and not in transition. The reason I did this was to simplify the conditions on scroll, resize and button click events that run every time.

    I introduced a menuVisible variable which will be false if the menu is not showing and not in transition. The reason I did this was to simplify the conditions on scroll, resize and button click events that run every time.

  • Fluent syntax for $(window) when hooking up events, saves converting window into a jQuery object twice.

    Fluent syntax for $(window) when hooking up events, saves converting window into a jQuery object twice.

  • Refactored a little so the functions are better split up and to clean up document ready.

    Refactored a little so the functions are better split up and to clean up document ready.

  • Something I noticed when playing with your menu from the link you gave, it's actually not very smooth at all on my desktop if the menu is transitioning at the same time as your slideshow. I suggest you halt the animation on your slideshow when the button is pressed, that will probably make the biggest difference to how smooth the page is.

    Something I noticed when playing with your menu from the link you gave, it's actually not very smooth at all on my desktop if the menu is transitioning at the same time as your slideshow. I suggest you halt the animation on your slideshow when the button is pressed, that will probably make the biggest difference to how smooth the page is.

    Alternatively you could disable the slideshow all together or remove the animation when it's in the mobile media query, scrolling may suffer as a result of the slideshow animation as well.

Alternatively you could disable the slideshow all together or remove the animation when it's in the mobile media query, scrolling may suffer as a result of the slideshow animation as well.

  • Ideally put your CSS in a separate file to keep your markup clean.
  • I put a function around your code so that the symbols can be properly minified.
  • I introduced a menuVisible variable which will be false if the menu is not showing and not in transition. The reason I did this was to simplify the conditions on scroll, resize and button click events that run every time.
  • Fluent syntax for $(window) when hooking up events, saves converting window into a jQuery object twice.
  • Refactored a little so the functions are better split up and to clean up document ready.
  • Something I noticed when playing with your menu from the link you gave, it's actually not very smooth at all on my desktop if the menu is transitioning at the same time as your slideshow. I suggest you halt the animation on your slideshow when the button is pressed, that will probably make the biggest difference to how smooth the page is.

Alternatively you could disable the slideshow all together or remove the animation when it's in the mobile media query, scrolling may suffer as a result of the slideshow animation as well.

  • Ideally put your CSS in a separate file to keep your markup clean.

  • I put a function around your code so that the symbols can be properly minified.

  • I introduced a menuVisible variable which will be false if the menu is not showing and not in transition. The reason I did this was to simplify the conditions on scroll, resize and button click events that run every time.

  • Fluent syntax for $(window) when hooking up events, saves converting window into a jQuery object twice.

  • Refactored a little so the functions are better split up and to clean up document ready.

  • Something I noticed when playing with your menu from the link you gave, it's actually not very smooth at all on my desktop if the menu is transitioning at the same time as your slideshow. I suggest you halt the animation on your slideshow when the button is pressed, that will probably make the biggest difference to how smooth the page is.

    Alternatively you could disable the slideshow all together or remove the animation when it's in the mobile media query, scrolling may suffer as a result of the slideshow animation as well.

Source Link
Daniel Imms
  • 985
  • 7
  • 16

Notes

I gave it a shot, there isn't really much that I felt could be improved upon though. I'm sure performance would be better if you were using CSS3 transitions (the implementation certainly would be nicer <3 CSS3) but I assume you wanted better browser coverage.

  • Ideally put your CSS in a separate file to keep your markup clean.
  • I put a function around your code so that the symbols can be properly minified.
  • I introduced a menuVisible variable which will be false if the menu is not showing and not in transition. The reason I did this was to simplify the conditions on scroll, resize and button click events that run every time.
  • Fluent syntax for $(window) when hooking up events, saves converting window into a jQuery object twice.
  • Refactored a little so the functions are better split up and to clean up document ready.
  • Something I noticed when playing with your menu from the link you gave, it's actually not very smooth at all on my desktop if the menu is transitioning at the same time as your slideshow. I suggest you halt the animation on your slideshow when the button is pressed, that will probably make the biggest difference to how smooth the page is.

Alternatively you could disable the slideshow all together or remove the animation when it's in the mobile media query, scrolling may suffer as a result of the slideshow animation as well.

Code

HTML

<div id="mobNav"></div>
<div id="shift">
 content Goes Here
</div>

CSS

#mobNav {
 background: url(images/top_bg.jpg) repeat;
 background-size: 100%;
 position:absolute;
 left:-200px;
 width:200px;
 height:100%;
}

JS

(function (,ドル window, document) {
 var mobNav, mainNav, top, shift;
 var menuVisible = false;
 
 $(document).ready(function($) {
 mobNav = $("#mobNav");
 mainNav = $("#mainnav");
 top = $("#top");
 shift = $("#shift");
 
 mobNav.html(mainNav.html());
 
 $(window).on("scroll", scroll);
 .on("resize", hideMenu);
 $("#mob_nav_trigger").click(function(e) {
 e.preventDefault();
 showMenu();
 });
 });
 
 function scroll() {
 if ($(window).scrollTop() > 300)
 top.fadeIn();
 else
 top.fadeOut();
 hideMenu();
 }
 
 function showMenu() {
 var winWidth = $(window).width(),
 docHeight = $(document).height();
 mobNav.css("minHeight", docHeight)
 .addClass("visible_now")
 .animate({ 
 left: menuVisible ? -mobNav.outerWidth() : 0 }, 
 function () { 
 if (menuVisible) 
 menuVisible = false;
 });
 shift.css("minWidth", winWidth)
 .animate({ marginLeft: menuVisible ? 0 : 200 });
 if (!menuVisible)
 menuVisible = true;
 }
 
 function hideMenu() {
 if (menuVisible) {
 mobNav.css("left", -mobNav.outerWidth());
 shift.css("margin-left", "0");
 menuVisible = false;
 }
 }
})(,ドル window, document);
default

AltStyle によって変換されたページ (->オリジナル) /