#6 created a Client-side functionality for the Helpful and Not Helpfu… #15

Yjohn · 2017-09-09T16:32:13Z

#6 it needs more work to make the buttons functional

…Helpful buttons

NateWr

Good start! Just a few things to touch up and then start working on the problem of how you'll get each question's button to work separately.

NateWr · 2017-09-11T09:18:47Z

views/index.hbs

+		<div class="entry__help">
+			<span id="displayCount">{{this.helpful}}</span>/<span id="totalNumber"></span> people found this helpful. Was this entry helpful?
+			<button id="Button1" class="button button--small">Yes</button>
+			<button id="Button2" class="button button--small">No</button>


Button1 and Button2 are bad names for the buttons. Consider using a more descriptive name like button-helpful and button-not-helpful.

NateWr · 2017-09-11T09:21:27Z

public/main.js

+    unhelpful++;
+    total++;
+    totalNumber.innerHTML = total;
+});


You're going to run into problems using getElementById, because every id on an HTML page must be completely unique. You can't have two elements with the same id.

Learn about querySelectorAll which allows you to select by class name and other attributes.

You'll face the same problem with your global helpful and total vars. If each question has it's own values, you'll need a way to store those values separately.

NateWr · 2017-09-11T09:21:56Z

views/layout.hbs

@@ -14,5 +14,6 @@
 			</header>
 			{{{body}}}
 		</div>
+    <script src="/main.js"></script>


The indentation doesn't look right here. Try formatting the document.

sending AJAX request to the route called 'rating' then resiving the change and display all the changes

…Helpful buttons

sending AJAX request to the route called 'rating' then resiving the change and display all the changes

Yjohn · 2017-09-14T21:14:01Z

I saw the review and changed a lot of staff

NateWr

Great work @Yjohn and @khaledkzy! Code looks really clean. Just a few comments throughout. I haven't tested this myself but let me know when you address the comments and I'll give it a go.

NateWr · 2017-09-15T09:22:16Z

public/main.js

+            helpful: helpful
+        })
+    };
+    fetch("/rating", options)


Where does the fetch function come from?

fetch is built-in in modern browsers (i can use fetch without using Jquery or any other library)

https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch

Cool! I didn't even know about that!

NateWr · 2017-09-15T09:22:21Z

public/main.js

+        })
+    };
+    fetch("/rating", options)
+        .then(response => response.json())


What's this line doing?

the route "/rating" is the Url that I and Khalid communicate with the server
it's the same like url: "/api/helpful", it's our argument

I mean specifically the line .then(response => response.json()) but I think I understand now. That's converting the response string into an object, yeah?

NateWr · 2017-09-15T09:24:52Z

routes/index.js

+  const renderEntries = function(error, entry) {
+    if (error) {
+      console.log(error);
+      res.sendStatus(400)


A 400 status usually indicates an authentication error (ie - "You are not allowed to do this."). In your case, I think you want to render a 500 status (ie - "The server ran into an error.").

NateWr · 2017-09-15T09:26:49Z

routes/index.js

+      {$inc: { [helpful ? 'helpful' : 'unhelpful' ]:  1}},
+      {new: true}
+    , renderEntries);
+});


Looks like you might have some indentation issues here. Try formatting the document.

NateWr · 2017-09-15T09:28:12Z

views/index.hbs

+			{{{this.answer}}}
+		</div>
+		<div class="entry__help">
+			<span class="value-helpful" data-question-id={{this._id}}>{{this.helpful}} people found this helpful. Was this entry helpful?</span>


Missing quotes " around {{this._id}}.

Also, you're missing the total count: 11/25 people found this helpful. You may need @khaledkzy to modify the data delivered to the template for you.

Khalid accept only the ID and the boolean of the Helpful buttons, and he changed depends on what he gets from me
if it's false he add the total number and if it's true he adds 1 to the helpful

Yes, but in this initial template, the string displays without a total count. You only say "11 people found this helpful" but you need to display "11/25 people found this helpful". Yeah?

NateWr · 2017-09-15T09:28:50Z

public/main.js

@@ -0,0 +1,32 @@
+const clickEvent = function (event) {
+    const id = event.target.getAttribute('data-question-id');
+    const helpful = JSON.parse(event.target.getAttribute('data-helpful'));


Do you need the JSON.parse() here?

yes, I need JSON.parse() to change the string to boolean

Yjohn · 2017-09-15T12:49:39Z

@NateWr I commented all the feedback, let me know ... :)

NateWr · 2017-09-15T13:04:33Z

@Yjohn Great! You can push new commits to this branch with changes for any outstanding issues (like this one).

NateWr

Nice work! The code looks really good. I haven't tested it yet but let me know when you make the last few changes requested and I'll take it for a spin.

NateWr · 2017-10-04T10:38:57Z

views/index.hbs

+			<span class="value-helpful" data-question-id={{this._id}}>{{this.helpful}}/{{this.total}} people found this helpful. Was this entry helpful?</span>
+			<button class="button button--small button-feedback" data-helpful="true" data-question-id="{{this._id}}">Yes</button>
+			<button class="button button--small button-feedback" data-helpful="false" data-question-id="{{this._id}}">No</button>
+			<div id="loading" class="hidden"></div>


The id attribute needs to be unique across the entire page. So if you have lots of elements with id="loading", it can cause problems if someone tries to use document.getElementById('loading').

It works in your case because you're limiting the selection document.querySelector('.class-<id> #loading'). But it should generally be avoided. You can either:

Add a unique value to each id: id="loading-{this._id}".

Turn the selector into a class: `class="hidden loading".

It's up to you.

I created an id only to apply the animation and style. If someone tries to use document.getElementById('loading'), only select empty <div> with animations.
I tried to make it class="hidden loading" but isn't working

Show me the code that you've tried to get it working with a class.

NateWr · 2017-10-04T10:44:29Z

public/main.js

+        let valueElId = valueEl.getAttribute('data-question-id')
+        if (res._id === valueElId) {
+            let totalNumber = res.helpful + res.unhelpful;
+            valueEl.innerHTML = res.helpful + '/' + totalNumber + ' people found this helpful. was this entry helpful';


Was this entry helpful?

This is how I received changes from the database and server, then display the number of people who select the button of helpful and the total number of people whose give their votes.
which part makes you confused?

Sorry, I should have explained. I meant to change the words at the end of the line from was this entry helpful to Was this entry helpful?

NateWr · 2017-10-15T19:20:03Z

This is amazing @Yjohn! I love the loading animation too. I've merged it now. 👍

NateWr#6 created a Client-side functionality for the Helpful and Not …

3a1775c

…Helpful buttons

NateWr suggested changes Sep 11, 2017

View reviewed changes

khaled and others added 10 commits September 14, 2017 18:30

Add the rating route

9a2df7b

Take the FS from the code

7190222

added the issues number NateWr#7

a0fc622

NateWr#6 creating client side functionality, listen all the buttons ...

4316157

sending AJAX request to the route called 'rating' then resiving the change and display all the changes

NateWr#6 created a Client-side functionality for the Helpful and Not …

a05f380

…Helpful buttons

NateWr#6 creating client side functionality, listen all the buttons ...

50e5cff

sending AJAX request to the route called 'rating' then resiving the change and display all the changes

fixing the function of receiveResponse

bbef6b1

change the name of not helpful to unhelpful

351e281

change from string to validate JSON object by using JSON.parse

5e50f6f

fixing the merge conflict

1ab3054

NateWr suggested changes Sep 15, 2017

View reviewed changes

Yjohn added 4 commits September 15, 2017 17:52

format the document

81d61c8

NateWr#6 display the total number of People

ea75504

change the variable name for total entiries

724f010

NateWr#6 adding loading animation when the button clicked

53cc818

NateWr suggested changes Oct 4, 2017

View reviewed changes

Yjohn added 2 commits October 6, 2017 18:52

change the attributes to be displayed

99bd9e5

NateWr#6 change the loading ID to the class

afb3936

NateWr merged commit 6dd75bf into NateWr:master Oct 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#6 created a Client-side functionality for the Helpful and Not Helpfu… #15

#6 created a Client-side functionality for the Helpful and Not Helpfu… #15

Yjohn commented Sep 9, 2017

NateWr left a comment

NateWr Sep 11, 2017

NateWr Sep 11, 2017

NateWr Sep 11, 2017

Yjohn commented Sep 14, 2017

NateWr left a comment

NateWr Sep 15, 2017

Yjohn Sep 15, 2017

Yjohn Sep 15, 2017

NateWr Sep 15, 2017

NateWr Sep 15, 2017

Yjohn Sep 15, 2017

NateWr Sep 15, 2017

NateWr Sep 15, 2017

NateWr Sep 15, 2017

NateWr Sep 15, 2017

Yjohn Sep 15, 2017

NateWr Sep 15, 2017

NateWr Sep 15, 2017

Yjohn Sep 15, 2017

NateWr Sep 15, 2017

Yjohn commented Sep 15, 2017

NateWr commented Sep 15, 2017

NateWr left a comment

NateWr Oct 4, 2017

Yjohn Oct 5, 2017

NateWr Oct 12, 2017

Yjohn Oct 13, 2017

NateWr Oct 4, 2017

Yjohn Oct 5, 2017

NateWr Oct 5, 2017

NateWr commented Oct 15, 2017

#6 created a Client-side functionality for the Helpful and Not Helpfu… #15

#6 created a Client-side functionality for the Helpful and Not Helpfu… #15

Conversation

Yjohn commented Sep 9, 2017

NateWr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yjohn commented Sep 14, 2017

NateWr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yjohn commented Sep 15, 2017

NateWr commented Sep 15, 2017

NateWr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NateWr commented Oct 15, 2017