Skip to content

add sound+animation, also some cleanup#1

Open
ludvigsj wants to merge 1 commit intomasterfrom
sound
Open

add sound+animation, also some cleanup#1
ludvigsj wants to merge 1 commit intomasterfrom
sound

Conversation

@ludvigsj
Copy link
Member

The following changes were made:

  • Add sensible .gitignore
  • Move css and js to own files
  • Add "GONG!"-animation when gong is clicked
  • Play gong sound when gong is clicked

@ludvigsj ludvigsj requested a review from tellnes May 10, 2017 16:39
Copy link
Contributor

@tellnes tellnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

La inn noen kommentarer jeg så kjapt. Har ikke sett gjennom style filen.

Deployer når ting er merget til master.

@@ -1 +1,79 @@
/node_modules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Joda, denne filen er generert. Men storparten her er da totalt unødvendig for oss. Er det ikke bedre å legge inn ting etterhvert som de kommer i veien?

Jeg hadde en slik global fil før, men har sluttet med det og legger nå bare inn enkelt linjer her og der.

</div>
<a id="lastgong">
</a>
<audio src="gong.mp3" id="lyden" preload >
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ekstra space på slutten

<a id="lastgong">
</a>
<audio src="gong.mp3" id="lyden" preload >
Your browser does not support audio
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dette kan vi vel ha på norsk. Eventuelt bare droppe lyd og tilbakemelding i det tilfellet.

@@ -0,0 +1,89 @@
var username = "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global variabel. Enkleste er å wrape hele filen i en funksjon.

if(req.readyState === XMLHttpRequest.DONE && req.status === 200) {
var res = JSON.parse(req.responseText);
outp.innerHTML = "Siste gong: " + res.name + ", " + new Date(res.timestamp).toLocaleString();
outp.href = "https://maps.google.com/maps?q=loc:"+res.location.latitude+","+res.location.longitude;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inkonsistent styling på om det skal være space før/etter + eller ikke i forhold til linjen over.

lyd.currentTime = 0;
lyd.play();
gongtext.className = "shown";
setTimeout(function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inkonsistent styling. De fleste andre steder i filen har du ikke space før { i funksjonsdeklarasjonen.

gongtext.className = "";
}, 2000);

navigator.geolocation.getCurrentPosition(function (pos) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto. Her har du også space etter function og før (.

navigator.geolocation.getCurrentPosition(function (pos) {
sendGong(pos.coords.latitude, pos.coords.longitude, username);
}, function (error){

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vi bør sende gong selv om geo feiler. Bare uten cords.

}
}
req.open("get", "/gong/gong");
req.send()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semikolon ditto.

req.onreadystatechange = function() {
if(req.readyState === XMLHttpRequest.DONE && req.status === 200) {
var res = JSON.parse(req.responseText);
outp.innerHTML = "Siste gong: " + res.name + ", " + new Date(res.timestamp).toLocaleString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Veldig lang linje. Hva med en maks linje lengde regel? F.eks. maks 80 tegn per linje.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants