Verwenden eines Navigationsmenüs, um das Material organisiert zu halten

1088
Andrew

Ich arbeite an einem persönlichen Projekt, um verschiedene Ausschnitte / Beispiele / kleine Projekte von mir organisiert zu halten. Ich möchte meine Seitenbreite optimal nutzen, also habe ich beschlossen, ein Navigationsmenü zu schreiben, das herausschiebt. Es funktioniert wie erwartet, aber der Code ist ziemlich groß.

Ich habe das HTML / CSS nur für den Fall bereitgestellt, dass das hilft, aber ich suche wirklich nur nach Hilfe beim JavaScript. Ich brauche niemanden, der das Ganze neu schreibt.

$(document).ready(function() {
	$('#tab').click(function() {
		if($('#main').attr('class') != 'out') {
			  $('#main').animate({
						'margin-left': '+=340px',
					  }, 500, function() {
						 $('#main').addClass('out');
					  });
												
				} else {
						$('#main').animate({
							'margin-left': '-=340px',
						  }, 500, function() {
							$('#main').removeClass('out');
						  });
						
					}
				});
				
				//does the same as above, but when I press ctrl + 1. from /http://www.scottklarr.com/topic/126/how-to-create-ctrl-key-shortcuts-in-javascript/
				$(document).keyup(function (e) {
					if(e.which == 17) isCtrl=false;
						}).keydown(function (e) {
							if(e.which == 17) isCtrl=true;
							if(e.which == 97 && isCtrl == true) {
								if($('#main').attr('class') != 'out') {
									  $('#main').animate({
												'margin-left': '+=340px',
											  }, 500, function() {
												 $('#main').addClass('out');
											  });
																		
										} else {
												$('#main').animate({
													'margin-left': '-=340px',
												  }, 500, function() {
													$('#main').removeClass('out');
												  });
												
											}
												return false;
											}
				});
			});
#sidebar { 	
	top: 0;
	left: 0;
	position: relative;
	float: left;
	}
	
#main {
	width: 320px;
	padding: 10px;
	float: left;
	margin-left: -340px;
	}

#tab {
	width: 30px;
	height: 120px;
	padding: 10px;
	float: left;
	}

.clear { clear: both; }
<div id="sidebar">
		<div id="main"></div>
		<div id="tab"></div>
		<br class="clear">
	</div>

Antworten
15
Ich würde empfehlen, dass Sie immer mit jslint.com beginnen, wodurch Ihre nicht deklarierte Variable und die in der Antwort angegebene braceless-Steueranweisung abgefangen wurden. citadelgrad vor 9 Jahren 4
Try Google's own Closure Compiler from Google http://code.google.com/closure/compiler/, which is a tool for making JavaScript download and run faster. It parses your JavaScript, analyzes it, removes dead code and rewrites and minimizes what's left. It also checks syntax, variable references, and types, and warns about common JavaScript pitfalls. There's a web version too: http://closure-compiler.appspot.com and a Linter. David d C e Freitas vor 9 Jahren 0

3 Antworten auf die Frage

12
PleaseStand

Erstens scheint es, dass isCtrlnirgendwo in Ihrem Code deklariert wird. Setzen Sie var isCtrl = false;als erste Zeile die Ready-Funktion ein. Andernfalls erhalten Sie einen JavaScript-Fehler, wenn der erste Schlüssel, den der Benutzer drückt, nicht der CtrlSchlüssel ist. Auch == trueist innerhalb einer ifAussage unnötig ; es kann weggelassen werden.

Zweitens könnte diese Zeile verbessert werden, da sie fehlschlägt, wenn das Element Mitglied einer anderen Klasse ist:

if($('#main').attr('class') != 'out') {

Es kann umgeschrieben werden .hasClass()als:

if(!$('#main').hasClass('out')) {

Sie sollten den gesamten Block in eine separate Funktion umgestalten, um das Duplizieren von Code zu vermeiden:

function toggleMenu() {
    if(!$('#main').hasClass('out')) {
        // ...
    } else {
        // ...
    }
}

Und legen Sie fest, toggleMenu();wo der doppelte Code in jeder Funktion war. Wenn die Leistung von Belang ist, sollten Sie das jQuery-Objekt zwischenspeichern, $('#main') indem Sie am Anfang der ready-Funktion eine andere Variable deklarieren. Dies ist langsam, da jQuery die übereinstimmenden Elemente im Dokument erneut finden müsste.

Ein kleinerer Kritikpunkt ist die Behauptung von Kontrollbefehlen, wie zum Beispiel:

if(e.which == 17) isCtrl=false;

Einige sagen, dass es am besten ist, den Code immer in eingerückte geschweifte Klammern zu setzen, um Fehler zu vermeiden, die durch falsch platzierte Semikolons verursacht werden.

Schließlich sieht die Einrückung im gesamten Code nicht richtig aus. Beheben Sie das, und Ihr Code wird lesbarer.

Hinzufügen: Es sieht auch so aus, als hätten Sie am Ende ein zusätzliches Komma:

'margin-left': '+=210px',

Dies ist syntaktisch gültig, verursacht jedoch Probleme mit Internet Explorer 7 und darunter .

Danke für die Vorschläge! Meine Frage wurde mit dem neuen [und verbesserten] Code aktualisiert. Danke auch für den Link zu dieser Performance-Seite. Ich habe den Verkettungstipp verwendet, um die Klassen zusätzlich zu der Sache "jQuery-Objekt als Variable" hinzuzufügen / zu entfernen. Ist es in Ordnung, die `function {...}` herauszuholen, die direkt nach den `500` in` .animate () `war? Es funktioniert, aber ich weiß nicht, ob das heißt, ich sollte es weglassen. Wie auch immer - danke nochmal! :) Andrew vor 9 Jahren 0
@Andrew: Der Hauptunterschied besteht darin, dass der `function {...}` Part erst ausgeführt wird, * nachdem * die Animation abgeschlossen ist. `.addClass ()` wurde nicht zur Warteschlange 'fx' hinzugefügt, daher wird das Verhalten anders sein. PleaseStand vor 9 Jahren 0
@Andrew: Deine neue Version des Codes sieht viel besser aus, obwohl es immer noch einige unnötige Nachkommas gibt, die ich nicht bemerkt habe. Ich weiß nicht, wie man am besten mit den verschiedenen Versionen umgeht. Vielleicht könnten Sie einen Einblick in [diese Meta-Frage] (http://meta.codereview.stackexchange.com/questions/41/iterative-code-reviews-how-can-they-happen-erfolgreich) geben. PleaseStand vor 9 Jahren 0
Die Kommas wurden korrigiert (wodurch 4 weitere Zeilen gelöscht wurden.) Der Code in der Frage wurde aktualisiert. Auch diese Meta-Frage bringt einen guten Punkt. So etwas hätte hier geholfen. Andrew vor 9 Jahren 0
6
ringzhz

der Rat von idealmachine ist ziemlich gesund. Ein weiterer kleiner Kritikpunkt ist die Verwendung von ifs zum Festlegen von booleschen Werten.

if(e.which == 17) { isCtrl = false; }

Sie können das etwas eleganter als umschreiben

isCtrl = e.which != 17;
4
ICR

Sie müssen toggleMenu nicht in eine Funktion einschließen. Sie können es einfach verwenden

$('#tab').click(toggleMenu);

Wenn Sie die Klammern nicht einschließen, bedeutet dies eher einen Verweis auf die Funktion als einen Funktionsaufruf.

Es ist keine gute Idee, die Tastaturmodifizierer manuell zu verfolgen. Wenn zum Beispiel Ihre Website den Fokus verliert, während die Taste gedrückt gehalten wird, wird sie denken, dass sie immer noch inaktiv ist, wenn Sie den Fokus erneut anzeigen, selbst wenn dies nicht der Fall ist.

Das Tastaturereignis hat eine ctrlKeyEigenschaft, die truelautet, wenn die Steuertaste gedrückt wird.

Es ist wahrscheinlich auch eine gute Idee, den Schlüsselcode in eine Variable zu packen, die beschreibt, um welchen Schlüssel es sich handelt, da das Erinnern an die 97Karten schwierig sein kann.

$(document).keydown(function (e) {
    if (e.ctrlKey && e.which === aKey) {
        toggleMenu();
        return false;
    }
});

Schließlich empfiehlt es sich, den Versatz in eine Variable zu extrahieren.

var menuSize = '210px';
var $main = $("#main"); 

function toggleMenu() {
    if(!$main.hasClass('out')) {
        $main.animate({'margin-left': '+=' + menuSize}, 500).addClass('out');                         
    } else {
        $main.animate({'margin-left': '-=' + menuSize}, 500).removeClass('out');
    }
}