Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add sound+animation, also some cleanup #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add sound+animation, also some cleanup #1

wants to merge 1 commit into from

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

</div>
<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