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 files via upload #6

Closed
wants to merge 1 commit into from
Closed

Conversation

fcoadebez
Copy link

Exercice ECV

function createTweetOl(tweet) {
var ol = document.createElement('ol');
var el = document.querySelector('div');
el.appendChild(ol);
Copy link
Owner

Choose a reason for hiding this comment

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

These two lines seem useless

Copy link
Owner

Choose a reason for hiding this comment

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

oh wait no, they're not useless, but they're not what was asked for.
This function should return an <ol> and the calling context should be the one deciding where the ol should be appended.

if (tweet.lang === "fr") {
li.classList.add("fr");
} else {
li.classList.add("en");
Copy link
Owner

Choose a reason for hiding this comment

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

You don't need that part, but if you really want it, prefer the lang attribute.

@@ -0,0 +1,12 @@
"use strict";

function createTweetOl(tweet) {
Copy link
Owner

Choose a reason for hiding this comment

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

This function creates an <ol> which only contains a single tweet.
It should take an array of tweets as argument and return an <ol> with one <li> per tweet

var el = document.querySelector('div');

tweets.forEach(function(tweet) {
createTweetOl(tweet);
Copy link
Owner

Choose a reason for hiding this comment

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

Related to above comment. There should not be one ol per tweet, but rather a single ol. Also, it's in the calling context that should be decided where the ol is appended.

<output></output>
</body>
<body>
<button id="buttonTweets" >Display French Tweets</button>
Copy link
Owner

Choose a reason for hiding this comment

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

The body should remain empty. Create it in JavaScript.


document.getElementById('buttonTweets').addEventListener("click", function() {
onlyFr = !onlyFr;
var liFr = document.querySelectorAll(".en");
Copy link
Owner

Choose a reason for hiding this comment

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

I'll take a longer time on this in class, but you shouldn't have to read information from the DOM. Instead, you should be re-creating new elements from the data (and only read the information to generate the elements from the data).

button.textContent = text;

// Add new button to body
body.appendChild(button);
Copy link
Owner

Choose a reason for hiding this comment

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

This function should only create the element, not append it to the body.

Copy link
Owner

Choose a reason for hiding this comment

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

The calling context should be deciding where the element is appended.

var mouseDownTimeStamp, mouseUpTimeStamp, clickTime;

Array.from(listButtonFat).forEach(function(button) {
button.addEventListener("mouseup", function(result) {
Copy link
Owner

Choose a reason for hiding this comment

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

All these event listeners belong in the createClickTimeButton function

********************** LISTENERS FAT BUTTON **********************
******************************************************************/

var listButtonFat = document.getElementsByClassName('buttonFat'), timer, num = 0;
Copy link
Owner

Choose a reason for hiding this comment

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

You shouldn't have to read from the DOM.

@DavidBruant
Copy link
Owner

Corrige les différents points en commentaires que je puisse valider

@DavidBruant
Copy link
Owner

(d'ici la semaine prochaine)

@DavidBruant
Copy link
Owner

Rien n'est corrigé -_-#

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

Successfully merging this pull request may close these issues.

2 participants