Reviews #523

jjeffryes · 2017-06-21T20:41:57Z

This adds the review display section to the listings details modal.

This PR also adds the average rating and number of ratings icons to the listing cards.

Closes #465

Rounded the average for the ratings.

…views # Conflicts: # js/languages/en-US.json

…views # Conflicts: # js/languages/en-US.json # js/views/modals/listingDetail/Listing.js

morebrownies · 2017-06-21T21:05:37Z

Nice, will review soon.

morebrownies · 2017-06-21T22:31:35Z

Client doesn't load for me on this branch, running into this error in the JS console:

…views # Conflicts: # js/languages/en-US.json # js/views/modals/listingDetail/Listing.js

jjeffryes · 2017-06-22T00:04:51Z

@morebrownies pull latest, I forgot to commit a missing comma I fixed earlier today.

morebrownies · 2017-06-22T19:09:32Z

Ok I found a few issues for you to revisit when you're back from vacation @jjeffryes

The user card on the about screen does not overall store rating / number of ratings.
The top bar should also display the overall store rating / number of ratings (next to "No location").
I changed the format of how to output number of ratings, it should be a super simple fix. Let's remove the Chat bubble icon and display number of reviews in parentheses, as pictured below. Ratings should display like this in every location:
Rating not displaying the same between store view and search view. Store view displays 4.0, search view displays 4.2. Search view also not displaying the number of reviews.
If there are 0 reviews on a listing, ⭐ 0 (0) should display under the BUY NOW button. Currently nothing displays and the review section further down the listing detail screen also is hidden.
The followers tab also doesn't display the number of ratings

rmisio · 2017-06-26T22:24:44Z

The first review is not showing the timestamp. It doesn't have a name (I don't think I checked the box), but there is a timestamp in the data:

rmisio · 2017-06-26T22:30:50Z

Should the expanded review really have an ellipse?

In this case it was so long, it looks like the server truncated it, but since the client has no way of knowing this... how would it be able to put the ellipse in that case?

(my guess is if you made a review that was too long for the client's non expanded view, but short enough where the server didn't truncate, it would wrongly show the ellipse)

rmisio · 2017-06-26T21:17:52Z

js/collections/listing/Reviews.js

+    return new BaseModel({
+      ...attrs,
+    }, options);
+  }


Why not simplify this and just do?

return new BaseModel(attrs, options);

The syntax you're using is useful if you're merging something with the attrs, but since you're not, it just makes sense to pass in the full object.

rmisio · 2017-06-26T21:18:32Z

js/collections/listing/Reviews.js

+
+
+export default class extends Collection {
+  constructor(models = [], options = {}) {


Is this constructor really needed? Since you're not doing anything custom, removing it would have the same result as the code in place.

rmisio · 2017-06-26T21:45:25Z

js/views/modals/listingDetail/Listing.js

@@ -508,6 +549,11 @@ export default class extends BaseModal {
      this.setSelectedPhoto(this.activePhotoIndex);


You have two super.render()'s in the render() function of this view.

rmisio · 2017-06-26T21:53:47Z

js/views/modals/listingDetail/Listing.js

@@ -508,6 +549,11 @@ export default class extends BaseModal {
      this.setSelectedPhoto(this.activePhotoIndex);
      this.setActivePhotoThumbnail(this.activePhotoIndex);
      this.adjustPriceBySku();
+      this.$reviews = this.$('.js-reviews');
+
+      // get the ratings data, if any


This horse is probably dead at this point, but I think it's a very poor practice in 99% of the cases to make a fetch in render. You're coupling the rendering of your view to a server request... but I'll put that aside for now.

At the very least, please cancel this call both in remove() and in render() as well. You want to cancel it in render() because you may re-render (e.g. user clicks on the data changed pop-up before the initial call finishes and since you're kicking off a second call, you want to ignore the first one).

rmisio · 2017-06-26T22:04:27Z

js/views/modals/listingDetail/Listing.js

@@ -508,6 +549,11 @@ export default class extends BaseModal {
      this.setSelectedPhoto(this.activePhotoIndex);
      this.setActivePhotoThumbnail(this.activePhotoIndex);
      this.adjustPriceBySku();
+      this.$reviews = this.$('.js-reviews');
+
+      // get the ratings data, if any


Also, there's no error handling here. For example, if a user has no reviews, the endpoint will error with:

{ "success": false, "reason": "no link named 'index.json' under QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn" }

In the case of an error, the first argument to always is the xhr which you end up passing to onRatings() which subsequently bombs with:

I would think instead of always(), you would want to use done() and fail() so you could identify errors (not sure if we want to display them to the user or just not show reviews in that case - the latter may be fine for now, but ideally we handle it in a way that doesn't leave exceptions in the code.)

Not sure if you want to fight this battle... I've logged many server issue today, so I'm over it, but ideally the server would return a 200 and an empty array if there are no reviews instead of:

{ "success": false, "reason": "no link named 'index.json' under QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn" }

As it is now, there's really no way to distinguish between a user having no reviews and there being an error fetching them. We may be able to punt on this if we decide to both silently ignore errors and no reviews.

(but as soon as Wolf asks for a 'This listing has no reviews' placeholder punting is not an option)

rmisio · 2017-06-26T22:40:07Z

styles/components/_misc.scss

+}
+
+.orderCompleteEvent {


I guess this could be removed if it's being driven by the 'clickable' state.

rmisio · 2017-06-26T22:40:41Z

js/templates/ratingsStrip.html

      const selected = ob.curRating >= ob.maxRating - i;
  %>
-  <a class="ion-star ratingIcon js-ratingIcon <%= ob.iconClrClass %> <% if (selected) print('selected') %>"></a>
+  <!--<a class="ion-star ratingIcon js-ratingIcon <%= ob.iconClrClass %> <% if (selected) print('selected') %>"></a>-->


Could we remove the commented out line?

rmisio · 2017-06-26T22:42:25Z

styles/modules/modals/_listingDetail.scss

+              width: 50%;
+              padding-bottom: $padSm;
+
+              .emoji {


I guess this could go away since you're globally applying these styles in _misc.scss?

…views

jjeffryes · 2017-06-28T22:05:26Z

@morebrownies and @rmisio this has been updated, including dead horses.

morebrownies · 2017-07-03T17:50:30Z

I keep running into this error when loading a listing detail modal. Maybe it is because I'm attempting to view a listings on a dated server?

jjeffryes · 2017-07-03T17:52:49Z

@morebrownies there have been several updates, it's possible this doesn't work with old nodes any more. I'll look into it.

…views # Conflicts: # js/languages/en-US.json

jjeffryes · 2017-07-03T20:06:18Z

@morebrownies you can retest now, make sure you're using servers updated in the last 30 minutes or so.

rmisio · 2017-07-03T20:16:16Z

js/views/modals/listingDetail/Listing.js

+        .fail((jqXhr) => {
+          if (jqXhr.statusText === 'abort') return;
+          // if there are no ratings, no response is returned.
+          if (jqXhr.status === 200) {


Is this if block really needed? I'm don't think there would ever be a case where a 200 triggers the fail handler.

…views # Conflicts: # js/languages/en-US.json

…views # Conflicts: # js/languages/en-US.json # styles/modules/modals/_orderDetail.scss

jjeffryes · 2017-07-06T20:56:59Z

@morebrownies this is just waiting for a final thumbs up from you to merge.

morebrownies · 2017-07-06T20:58:51Z

Awesome. I'll test it again shortly.

morebrownies · 2017-07-06T22:46:31Z

Ok still seeing lots of issues on this PR, but it may be my local server/client that is causing some of the issues.

Search results do not show the # of ratings, only the overall rating
The listing detail screen does not show the # of ratings or the overall rating
Total # of reviews shows 4 in the title bar, but it appears only 2 reviews were left on the listings within the store. This may be because a listing with reviews was deleted or maybe there is a bug? not sure, but worth calling out.
Can you make it so that the default message in the review section (No reviews were found for this listing) in the listing details modal displays in italics like the default messages in Refund Policy and Terms of Service display?
Can the reviews be right aligned in the user card? Currently there is a big gap on the right side.

That's all I've found so far

morebrownies · 2017-07-08T17:39:39Z

The format is also off in list view mode on a store

- Style the no reviews text the same as other no data text. - Push the ratings in the user card all the way to the right. - Apply the same changes to the ratings in list view. - Remove extra space after the star.

jjeffryes · 2017-07-10T17:05:32Z

@morebrownies this has been updated.

Notes:

Search Results - the seach data doesn't have the number of reviews yet. When Tyler adds that, they will show up.

Zero Ratings - this is fixed, it will show zeroes instead of blanks when there is no value.

Mismatch in ratings on the top bar: this is a server bug, I told @cpacia about it a while back, but it doesn't appear to be fixed. I have filed an issue at OpenBazaar/openbazaar-go#566

Italicize and color 2 on no reviews text is fixed.

Right align rating is fixed.

Formatting in the grid view is fixed.

The spacing after the star was reduced, it had an extra space.

morebrownies · 2017-07-10T18:07:47Z

Ok, great. Thanks for the update @jjeffryes

Is this a bug I'm seeing here? I noticed on @cpacia store in the grid view it shows the listing has 4 reviews, but when I click into the listing detail modal I do not see any.

ob://QmZXGKrRRe5RTDhvzhZfSTe8vYBDGV7HBoNz5FS3U9FnUa/store/piss

…ng rating data from options.

jjeffryes · 2017-07-10T18:09:59Z

@morebrownies that isn't a bug, Chris manually deleted his reviews while testing something, so that store has bad data.

morebrownies · 2017-07-10T18:11:35Z

Ok curious why it still shows reviews at the grid view level? Shouldn't it show the same thing between grid view and listing modal?

jjeffryes · 2017-07-10T18:16:19Z

@morebrownies he must have put them back, I get correct reviews on the listing now.

morebrownies · 2017-07-10T18:19:41Z

I see them now too, but the grid view and modal still aren't showing the same number of reviews and rating. Is this due to caching or something?

jjeffryes · 2017-07-10T18:39:39Z

@morebrownies I don't know, it's a server issue one way or another.

You can use the test store I set up, it's up to date and has clean data.

ob://QmYfqEPxcFqA1eTDx8vVeNkadLwWbSJpNzvhghDvsSSQG5/store/

…views # Conflicts: # js/languages/en-US.json

morebrownies · 2017-07-10T18:52:59Z

Ok cool. Other than some server weirdness, I think this is looking pretty good.

jjeffryes added 11 commits June 2, 2017 17:09

Initial commit of reviews.

ad48415

Wire in individual reviews.

707704f

Adjusted styling to the more/less links.

07ca4be

Rounded the average for the ratings.

Add ratings to listingCard

3d5b265

Change the Review model into a basic model.

7ea4313

Handle regions of "All"

672e3a4

Undo commit to wrong branch.

6e5ba7d

Merge remote-tracking branch 'refs/remotes/OpenBazaar/master' into re…

65d34fc

…views # Conflicts: # js/languages/en-US.json

Update reviews to use the same stars as the order complete section.

4bb0eed

Better way of setting clickable.

7237627

Merge remote-tracking branch 'refs/remotes/OpenBazaar/master' into re…

d681a57

…views # Conflicts: # js/languages/en-US.json # js/views/modals/listingDetail/Listing.js

jjeffryes requested review from rmisio and morebrownies June 21, 2017 20:42

Merge remote-tracking branch 'refs/remotes/OpenBazaar/master' into re…

1e26613

…views # Conflicts: # js/languages/en-US.json # js/views/modals/listingDetail/Listing.js

rmisio reviewed Jun 26, 2017

View reviewed changes

jjeffryes added 2 commits June 28, 2017 18:03

Review changes.

3a02b30

Merge remote-tracking branch 'refs/remotes/OpenBazaar/master' into re…

25c5fa0

…views

jjeffryes added 3 commits July 3, 2017 15:07

Recognize a 404 as no reviews are available.

b71a4d8

Remove 404 check, the server will return 200 now.

53f52c7

Merge remote-tracking branch 'refs/remotes/OpenBazaar/master' into re…

6625a74

…views # Conflicts: # js/languages/en-US.json

rmisio reviewed Jul 3, 2017

View reviewed changes

jjeffryes added 2 commits July 3, 2017 16:27

Merge remote-tracking branch 'refs/remotes/OpenBazaar/master' into re…

f126cdd

…views # Conflicts: # js/languages/en-US.json

Merge remote-tracking branch 'refs/remotes/OpenBazaar/master' into re…

381b29d

…views # Conflicts: # js/languages/en-US.json # styles/modules/modals/_orderDetail.scss

Review changesShow zeroes if the data is missing for ratings.

59d9d0b

- Style the no reviews text the same as other no data text. - Push the ratings in the user card all the way to the right. - Apply the same changes to the ratings in list view. - Remove extra space after the star.

Remove toFixed from listing which errors on undefined data, add setti…

a004212

…ng rating data from options.

Merge remote-tracking branch 'refs/remotes/OpenBazaar/master' into re…

b1acb56

…views # Conflicts: # js/languages/en-US.json

jjeffryes merged commit c7fec41 into OpenBazaar:master Jul 11, 2017

jjeffryes deleted the reviews branch July 11, 2017 18:57

snyk-bot mentioned this pull request Aug 30, 2022

[Snyk] Fix for 2 vulnerabilities phungmobile2/openbazaar-desktop#107

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reviews #523

Reviews #523

jjeffryes commented Jun 21, 2017 •

edited

Loading

morebrownies commented Jun 21, 2017

morebrownies commented Jun 21, 2017

jjeffryes commented Jun 22, 2017

morebrownies commented Jun 22, 2017

rmisio commented Jun 26, 2017

rmisio commented Jun 26, 2017 •

edited

Loading

rmisio Jun 26, 2017

rmisio Jun 26, 2017

rmisio Jun 26, 2017

rmisio Jun 26, 2017

rmisio Jun 26, 2017

rmisio Jun 26, 2017

rmisio Jun 26, 2017

rmisio Jun 26, 2017

rmisio Jun 26, 2017

jjeffryes commented Jun 28, 2017

morebrownies commented Jul 3, 2017

jjeffryes commented Jul 3, 2017

jjeffryes commented Jul 3, 2017

rmisio Jul 3, 2017

jjeffryes commented Jul 6, 2017

morebrownies commented Jul 6, 2017

morebrownies commented Jul 6, 2017

morebrownies commented Jul 8, 2017

jjeffryes commented Jul 10, 2017

morebrownies commented Jul 10, 2017

jjeffryes commented Jul 10, 2017

morebrownies commented Jul 10, 2017

jjeffryes commented Jul 10, 2017

morebrownies commented Jul 10, 2017

jjeffryes commented Jul 10, 2017

morebrownies commented Jul 10, 2017



		export default class extends Collection {
		constructor(models = [], options = {}) {

		@@ -508,6 +549,11 @@ export default class extends BaseModal {
		this.setSelectedPhoto(this.activePhotoIndex);

		}

		.orderCompleteEvent {

Reviews #523

Reviews #523

Conversation

jjeffryes commented Jun 21, 2017 • edited Loading

morebrownies commented Jun 21, 2017

morebrownies commented Jun 21, 2017

jjeffryes commented Jun 22, 2017

morebrownies commented Jun 22, 2017

rmisio commented Jun 26, 2017

rmisio commented Jun 26, 2017 • edited Loading

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

jjeffryes commented Jun 28, 2017

morebrownies commented Jul 3, 2017

jjeffryes commented Jul 3, 2017

jjeffryes commented Jul 3, 2017

Choose a reason for hiding this comment

jjeffryes commented Jul 6, 2017

morebrownies commented Jul 6, 2017

morebrownies commented Jul 6, 2017

morebrownies commented Jul 8, 2017

jjeffryes commented Jul 10, 2017

morebrownies commented Jul 10, 2017

jjeffryes commented Jul 10, 2017

morebrownies commented Jul 10, 2017

jjeffryes commented Jul 10, 2017

morebrownies commented Jul 10, 2017

jjeffryes commented Jul 10, 2017

morebrownies commented Jul 10, 2017

jjeffryes commented Jun 21, 2017 •

edited

Loading

rmisio commented Jun 26, 2017 •

edited

Loading