feedback på koding!

[Fylling]

Hei!

 

Eg har lyst å bli mykje betre i javaskript, så eg håpte at dere kunne "kjefte" på meg for det lille skriptet eg legger ut no?

Vil prøve å skrive veldig effektive skripts, så trenger feedback frå folk som kan dette og har gjerne års erfaring, eller berre ser ting eg gjer gale!

 

Tusen takk!

http://pastebin.com/Nx9dpBMY

 

 

Edit: gløymte å nemne at dette er koden eg skal bruke på nettsida mi. Ettersom nettsida bruker overflow:hidden og scroller automatisk til divs, trenger eg eit skript som setter ein id på valgt section (viser i menyen kor du befinner deg).

Endret 19. august 2013 av Fylling

[The Jackal]

Nå har jeg ikke supererfaring med akkurat javascript, men har jobbet som programmerer i ~10år. Første som slår meg i alle fall, er at du bruker en variabel for å holde state (var freshRun = true. Sånn generelt sett er ikke det god koding, så om du kan komme bort i fra det, tror jeg det ville vært bra. Nummer 2 så finnes det mekanismer i javascript som gjør at du slipper hacket for å kunne kalle "construct" metoden din. Om du skriver:

setSection.prototype.construct = function(topOffset,offsets) {
        var menuHeight = $('nav#main').height();
        var offset = $('nav#main').offset().top + menuHeight;
        if (topOffset != offset || (topOffset <= menuHeight+2 && freshRun == true) || freshRun == true) {
            freshRun = false;
            var sectionKey = getCurrentSection(offset, 1, offsets);//offset, margin, offsets
            setAttributes(sectionKey);
 
 
            $('#tull').html('x: ' + offset + '<br>margin: 3<br>min: ' + 0 + '<br>max: ' + $('section#tilbud').offset().top + '<br>section: ' + sectionsID[sectionKey]);
            lastSection = sectionsID[sectionKey];//sets the last section
        }
    }

så skal det være mulig å bare gjøre slik:

var section = new setSection();
section.construct(topOffset,offsets);

du kan da kunne droppe hele new Object() greia di.

Ellers ville i alle fall jeg gitt nytt navn til setSection() til bare Section(), altså i bunn og grunn konstruer det hele som klasse. Kikk litt på denne https://developer.mozilla.org/en-US/docs/Web/JavaScript/Introduction_to_Object-Oriented_JavaScript så skjønner du kanskje litt mer hva jeg vil fram til.

[Fylling]

Kan du gi eit tips om korleis eg kan fjerne freshRun?

[Fylling]

constructor blir berre kalt første gangen eg skriver:

var Section = new Section();

Isåfall kan eg enkelt fjerne freshRun variabellen, somtidig som eg kan korte ned koden eindel.

[Fylling]

http://pastebin.com/h19GBenN

 

Var ditta ein mykje betre måte? 0.o

 

Edit;

Eller kanskje: http://pastebin.com/Dv2cnU6Y

Endret 20. august 2013 av Fylling

[The Jackal]

Regner med du manglet litt kode i den første der? Den siste ser bedre ut enn det du lagde først i alle fall. I alle fall sett fra et objektorientert synspunkt. Føler du at du fikk ryddet opp litt?

 

Som sagt driver jeg ikke så fryktelig mye med javascript, men fant denne http://javascript.crockford.com/private.html som du også bør lese. Tror ikke jeg skal anbefale så mye mer utover dette egentlig, all den tid jeg stort sett sitter med C#. En som virkelig kan javascript får fortsette om det er andre ting som bør gjøres.

[Fylling]

Regner med du manglet litt kode i den første der? Den siste ser bedre ut enn det du lagde først i alle fall. I alle fall sett fra et objektorientert synspunkt. Føler du at du fikk ryddet opp litt?

 

Som sagt driver jeg ikke så fryktelig mye med javascript, men fant denne http://javascript.crockford.com/private.html som du også bør lese. Tror ikke jeg skal anbefale så mye mer utover dette egentlig, all den tid jeg stort sett sitter med C#. En som virkelig kan javascript får fortsette om det er andre ting som bør gjøres.

 

er ikkje heilt vant med oop, så det var litt tungt i går. Berre ditta lille der 0.o

[The Jackal]

Tror du over tid vil merke at du får større utbytte av koden din om du fortsetter i dette sporet i alle fall Ikke at du nødvendigvis skal gjøre absolutt alle javascript funksjoner som oop, men i alle fall vite om de forskjellige mulighetene og bruke det der det er naturlig.

[Fylling]

Dette fungerer perfekt... I firefox. I chrome fungerer det ikkje 0.o

http://pastebin.com/HL2zr1n7

 

Eg prøvde til og med å "rope" på metodene når metoden før var ferdig, men fekk same resultat.

http://pastebin.com/9tanmC3G

 

 

Nokon ideer? :/

 

 

 

Edit:

 

Prøvde litt "debugging":

http://pastebin.com/sX6wnnwT

 

Output:

Chrome:
Section set!
__construct set currentKey to: undefined
Setting attributes..
getIDString!
Attributes set!

Firefox:
Section set!
__construct set currentKey to: 0
Setting attributes..
getIDString!
Attributes set!

Begge har kronologisk output, alt stemmer med unntak av eine outputten i chrome:

__construct set currentKey to: undefined

Som skulle ha hatt ein verdi på 0, slik som i firefox. Nokon idear kvifor?

Endret 21. august 2013 av Fylling

[The Jackal]

Vel...slik koden din er skrevet, så er det ikke gitt at currentKey blir satt. Du kan altids initialisere Section klassen slik:

function Section() {
    this.currentKey=0;
    this.offsets=[];
    this.sectionID= ['heim', 'tilbud', 'galleri', 'feedback'];
    this.prefixPath='body>section#content>section#';
    $('div#tull').append('Section set!<br>');
}

Vet ikke om det fikser problemet, men det er uansett bra praksis å initialisere variabler.

Endret 21. august 2013 av The Jackal

[Fylling]

Vel...slik koden din er skrevet, så er det ikke gitt at currentKey blir satt. Du kan altids initialisere Section klassen slik:

function Section() {
    this.currentKey=0;
    this.offsets=[];
    this.sectionID= ['heim', 'tilbud', 'galleri', 'feedback'];
    this.prefixPath='body>section#content>section#';
    $('div#tull').append('Section set!<br>');
}

Vet ikke om det fikser problemet, men det er uansett bra praksis å initialisere variabler.

 

Det fungerte ikkje, nå står berre currentKey som 0 i chrome, kvar gang.

[The Jackal]

for (var i = 0; i < this.offsets.length; i++) {
        if (i + 1 != this.offsets.length) {
            if (this.topOffset >= this.offsets[i] && this.topOffset < this.offsets[i + 1]) {
                this.currentKey = i;
                break;
            }
        } else if (this.topOffset >= this.offsets[i]) {
            this.currentKey = i;
            break;
        }
    }

Det er strengt tatt her du må debugge. Hva er f.eks this.offsets.length, hvor mange ganger kjører loopen, hva er i, hva er this.topOffset. Uten å vite disse tingene er det litt vanskelig å si noe hva som faktisk er galt.

[Fylling]

for (var i = 0; i < this.offsets.length; i++) {
        if (i + 1 != this.offsets.length) {
            if (this.topOffset >= this.offsets[i] && this.topOffset < this.offsets[i + 1]) {
                this.currentKey = i;
                break;
            }
        } else if (this.topOffset >= this.offsets[i]) {
            this.currentKey = i;
            break;
        }
    }

Det er strengt tatt her du må debugge. Hva er f.eks this.offsets.length, hvor mange ganger kjører loopen, hva er i, hva er this.topOffset. Uten å vite disse tingene er det litt vanskelig å si noe hva som faktisk er galt.

 

 

Trur dette kan ha løst det!

 

Kode:

//constructor
Section.prototype.construct = function (topOffset) {
    this.topOffset = topOffset;

    //sets the offsets values
    for (var u = 0; u < this.sectionID.length; u++) {
        this.offsets[u] = $(this.prefixPath+this.sectionID[u]).offset().top;
    }
    
    //gets the currentKey
    $('div#tull').append('setter currentKey..' + '<br>');
    $('div#tull').append('currentKey default: ' + this.currentKEy + '<br>');
    $('div#tull').append('this.offsets.length: '+this.offsets.length+'<br>');
    for (var i = 0; i < this.offsets.length; i++) {
        $('div#tull').append('i: '+i + '<br>');
        if (i + 1 != this.offsets.length) {
            $('div#tull').append('first main if == true' + '<br>');
            if (this.topOffset >= this.offsets[i] && this.topOffset < this.offsets[i + 1]) {
                this.currentKey = i;
                $('div#tull').append('inner if == true' + '<br>');
                break;
            } else {
                $('div#tull').append(this.topOffset + ' >= ' + this.offsets[i] + ' && '+this.topOffset+' < '+this.offsets[i+1]+'<br>');
            }
        } else if (this.topOffset >= this.offsets[i]) {
            this.currentKey = i;
            $('div#tull').append('second main if == true' + '<br>');
            break;
        }
        
        $('div#tull').append('END OF ROUND<br>');
    }
    $('div#tull').append('currentKey set to: '+this.currentKey + '<br>');

    //$('div#tull').append('__construct set currentKey to: '+this.currentKey+'<br>');

    return false;
}

Output:

Section set!
setter currentKey..
currentKey default: undefined
this.offsets.length: 4
i: 0
first main if == true
65 >= 67 && 65 < 552
END OF ROUND
i: 1
first main if == true
65 >= 552 && 65 < 1442
END OF ROUND
i: 2
first main if == true
65 >= 1442 && 65 < 2332
END OF ROUND
i: 3
END OF ROUND
currentKey set to: 0
Setting attributes..
getIDString!
Attributes set!

Så nå ser eg at meny høgda er 2px lavere i chrome enn firefox. merklig, men får fikse dette og håpe det funker!

 

 

Løysning:

var topOffset=$('nav#main').offset().top+$('nav#main').height();
if(window.chrome){topOffset+=2;}

Endret 22. august 2013 av Fylling

[Fylling]

Fullstendig og ferdig kode!

 

Javascript.js

//******************************************************************************************************
//finds section by range in heights 

function Section() {
    this.currentKey = 0;
    this.offsets = [];
    this.sectionID = ['heim', 'tilbud', 'galleri', 'feedback'];
    this.prefixPath = 'body>section#content>section#';
    this.menuHeight = $('nav#main').height();
    if (window.chrome) { this.menuHeight += 2; }
    this.oL;
    this.o;
    this.lastSection;
    $('div#tull').append('Section Class activated <br><br>');
}

//constructor
Section.prototype.construct = function (topOffset) {
    this.topOffset = topOffset;

    //sets the offsets values
    for (var u = 0; u < this.sectionID.length; u++) {
        this.offsets[u] = $(this.prefixPath+this.sectionID[u]).offset().top;
    }
    this.oL = this.offsets.length;

    //gets the currentKey
    for (var i = 0; i < this.oL; i++) {
        this.tO = this.topOffset+this.menuHeight;
        this.o = this.offsets[i];
        if (i + 1 != this.oL) {
            if (this.tO >= this.o && this.tO < this.offsets[i + 1]) {
                this.currentKey = i;
                break;
            }
        } else if (this.tO >= this.o) {
            this.currentKey = i;
            break;
        }
    }
    return false;
}

//change attribute
Section.prototype.changeIDString = function () {
    this.top = $('body>nav#main').offset().top + this.menuHeight;
    for (var i = 0; i < this.oL; i++) {
        this.o = this.offsets[i];
        if (i + 1 != this.oL) {
            if (this.top >= this.o && this.top < this.offsets[i + 1]) {
                this.currentKey = i;
                break;
            }
        } else if (this.top >= this.o) {
            this.currentKey = i;
            break;
        }
    }
    this.lastSection = this.id;
    return false;
}

//get current
Section.prototype.getIDString = function () {
    return this.sectionID[this.currentKey];
}

Section.prototype.checkString = function (virgin) {
    this.virgin = virgin;
    if(this.virgin){return false;}
    this.top = $('body>nav#main').offset().top + this.menuHeight;

    if (this.currentKey + 1 != this.oL) {
        if (this.top >= this.offsets[this.currentKey] && this.top < this.offsets[this.currentKey + 1]) { return true; }
    } else {
        if (this.top > this.offsets[this.currentKey]) { return true;}
    }
    return false;
}

//set classes
Section.prototype.setAttributes = function (virgin) {
    if (this.checkString(virgin)) { return false; }
    this.changeIDString();
    this.id = this.getIDString();
    $('#current').removeAttr('id');
    $('[data-gotoSection="#' + this.id + '"]').attr('id', 'current');
    $('div#tull').append('Forandret!<br>');
    return false;
}

var Section = new Section();

footer.php

	<!-- javaskript -->
	<script src="js/jquery.js"></script>
	<script src="js/foundation.min.js"></script>
	<script src="http://cdnjs.cloudflare.com/ajax/libs/gsap/latest/TweenMax.min.js"></script><!--CDN link for the latest TweenMax-->
	<script src="http://snorkl.tv/dev/js_demo1/js/greensock/plugins/ScrollToPlugin.js"></script>
	<script src="js/res.2.min.js"></script>
	<script src="js/md5.js"></script>
	<script src="js/javaskript.js"></script>
	
	<!-- JQuery -->
	<script type="text/javascript">
		$(document).ready(function(){
			/** Variablar **/
			var HDHeightArray = ['.tile','.HDheight'];
			var topOffset=$('nav#main').offset().top;
			
                        [...]

			//find the section
			Section.construct(topOffset);
			//Section.changeIDString();
			Section.setAttributes(true);
			window.setInterval(function(){Section.setAttributes(false);},100);//markerer aktive section
			[...]
		});
	</script> 
</body>
</html>

Stor fornøgd!

Nå må eg berre passe på at this.offsets blir generert på ny om nettleser høgda forandres. Ellers så har eg skrevet det litt om slik at den ikkje køyrer for loops kvar 50ms om menyen ikkje "toucher" ein ny section, og da skal den forandres. YAY!