Allow users to edit their reviews #3220

kumar303 · 2017-09-22T21:38:43Z

I focused mainly on mobile styles only since we will do desktop styles in mozilla/addons#10472

Here is the new edit flow. A key difference is that the stars are now editable in the overlay.

(The blue line at the end is just a glitch in how I converted the movie.)

… work

kumar303 · 2017-09-22T21:43:51Z

package.json

@@ -244,7 +244,7 @@
    "intl": "^1.2.5",
    "intl-locales-supported": "^1.0.0",
    "jest": "^21.0.2",
-    "jest-enzyme": "^3.2.0",
+    "jest-enzyme": "^3.8.3",


This fixed a bug I was seeing in expect()

I definitely worked around these in #3202 so the tests aren't as good in there... thanks!

kumar303 · 2017-09-22T21:56:58Z

src/amo/components/AddonReview/index.js

@@ -179,7 +200,7 @@ export class AddonReviewBase extends React.Component {
          <input
            className="AddonReview-submit"
            type="submit"
-            value={i18n.gettext('Submit review')}
+            value={i18n.gettext('Update review')}


It's pretty much always an update whenever you're on this screen. Even the first time you see it, you are editing the review since you have already selected a star rating to get there.

Is there any (easy) way to check this though? Like maybe if the existing review text is zero-length? I get that technically they're updating a review after clicking a star, but we call that a rating and it doesn't feel like it. It would be a lot nicer UX to keep "Submit review" for the first time.

There's not an easy way. I would have to make the internal state of RatingManager more complex and that test suite doesn't test the click-star-then-edit flow in its entirety (due to react-test-utils limitations).

I'll just revert it back to Submit review for now since that makes sense in either case.

kumar303 · 2017-09-22T22:05:17Z

src/amo/components/AddonReviewListItem/index.js

+  onEscapeReviewOverlay = () => {
+    // Even though an escaped overlay will be hidden, we still have to
+    // synchronize our show/hide state otherwise we won't be able to
+    // show the overlay after it has been escaped.


This is a bit lame since we have a chain of callbacks now. I explained the issue in https://github.com/mozilla/addons-frontend/issues/3222 but I think using callbacks is a quicker fix for now.

kumar303 · 2017-09-22T22:09:06Z

tests/unit/amo/components/TestAddonReviewList.js

+      store,
+      ...customProps,
+    };
+    return shallowUntilTarget(


Believe it or not I didn't want to spend time converting this whole suite to shallow rendering. However, since I had to refactor the reviews state and be sure I didn't break things along the way, it was really helpful to switch to shallow rendering first. It also turned out to be a nice validation of this shallowUntilTarget pattern because you can see how this component's reliance on mapped state is fully tested now.

Yeah I really like testing the entire component including the HOC. I'm happy with have that pattern.

codecov-io · 2017-09-22T22:48:32Z

Codecov Report

Merging #3220 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    mozilla/addons-frontend#3220      +/-   ##
==========================================
+ Coverage    95.4%   95.41%   +<.01%     
==========================================
  Files         159      160       +1     
  Lines        2984     3011      +27     
  Branches      588      593       +5     
==========================================
+ Hits         2847     2873      +26     
- Misses        118      119       +1     
  Partials       19       19

Impacted Files	Coverage Δ
src/ui/components/OverlayCard/index.js	`100% <ø> (ø)`	⬆️
src/amo/components/AddonReviewList/index.js	`97.91% <100%> (ø)`
src/ui/components/Overlay/index.js	`100% <100%> (ø)`	⬆️
src/amo/reducers/reviews.js	`100% <100%> (ø)`	⬆️
src/amo/components/AddonReview/index.js	`100% <100%> (ø)`
src/amo/components/AddonReviewListItem/index.js	`100% <100%> (ø)`
src/amo/components/Addon/index.js	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3921ae...bf55d18. Read the comment docs.

tofumatt

Code looks good but I think the UX of this is a bit off. I think changing the button to more of a link style and giving it more context would be best.

Works well though, so once that's done I think it's set to merge!

tofumatt · 2017-09-23T18:29:40Z

package.json

@@ -244,7 +244,7 @@
    "intl": "^1.2.5",
    "intl-locales-supported": "^1.0.0",
    "jest": "^21.0.2",
-    "jest-enzyme": "^3.2.0",
+    "jest-enzyme": "^3.8.3",


I definitely worked around these in #3202 so the tests aren't as good in there... thanks!

tofumatt · 2017-09-23T18:29:51Z

src/amo/components/AddonReview/index.js

 import type { SetReviewAction, UserReviewType } from 'amo/actions/reviews';
 import type { SubmitReviewParams } from 'amo/api/index';
 import type { ApiStateType } from 'core/reducers/api';
 import type { ErrorHandler as ErrorHandlerType } from 'core/errorHandler';
 import type { ElementEvent } from 'core/types/dom';
 import type { DispatchFunc } from 'core/types/redux';

-import 'amo/css/AddonReview.scss';
+import './styles.scss';


Thanks for tidying this up 😄

tofumatt · 2017-09-23T18:32:08Z

src/amo/components/AddonReview/index.js

@@ -179,7 +200,7 @@ export class AddonReviewBase extends React.Component {
          <input
            className="AddonReview-submit"
            type="submit"
-            value={i18n.gettext('Submit review')}
+            value={i18n.gettext('Update review')}


Is there any (easy) way to check this though? Like maybe if the existing review text is zero-length? I get that technically they're updating a review after clicking a star, but we call that a rating and it doesn't feel like it. It would be a lot nicer UX to keep "Submit review" for the first time.

tofumatt · 2017-09-23T18:33:04Z

src/amo/components/AddonReview/styles.scss

  resize: vertical;
+
+  @include respond-to("medium") {


You don't actually need the quotes here and we've tended not to use them. I only mention it because it'll make it easier to grep for if we're consistent. I'm happy to go all-quotes too, but right now we tend not to use them.

I get that's confusing though.

tofumatt · 2017-09-23T18:34:02Z

src/amo/components/AddonReviewList/index.js

@@ -55,6 +52,8 @@ export class AddonReviewListBase extends React.Component {
  }

  loadDataIfNeeded(nextProps?: AddonReviewListProps) {
+    const lastAddon = this.props.addon;


No const { addon: lastAddon } = this.props;? I think I'm finally used to that syntax! 😆

It only seems necessary to me if you need to destruct multiple variables at once. Otherwise, what's the point?

Fair enough.

tofumatt · 2017-09-23T18:50:26Z

tests/unit/amo/components/TestAddonReviewListItem.js

+  let store;
+
+  beforeEach(() => {
+    store = createStore().store;


Again, dispatchSignInActions() might be better.

tofumatt · 2017-09-23T18:51:10Z

tests/unit/amo/components/TestAddonReviewListItem.js

+    expect(rating).toHaveProp('readOnly', true);
+  });
+
+  it('renders new lines in review bodies', () => {


Nitpick but I think it's "newlines", no?

tofumatt · 2017-09-23T18:53:25Z

tests/unit/amo/components/TestAddonReviewListItem.js

+      .find(LoadingText)).toHaveLength(1);
+  });
+
+  it('does not render controls when signed in without a review', () => {


I found this spec a bit hard to parse. I got what it meant but I think it might be better rephrased:

it('does not render review controls unless the user left a review', () => {

tofumatt · 2017-09-23T18:53:54Z

tests/unit/amo/components/TestAddonReviewListItem.js

+    expect(root.find('.AddonReviewListItem-controls')).toHaveLength(0);
+  });
+
+  it('does not render controls for the wrong user', () => {


Maybe "does not render controls for another user's review"?

tofumatt · 2017-09-23T19:10:02Z

src/amo/components/AddonReviewListItem/index.js

+              onClick={this.onClickToEditReview}
+              className="AddonReviewListItem-edit-button Button--action Button--small"
+            >
+              {i18n.gettext('Edit')}


This button could definitely use some more context. I also don't really think it should be a button. I think stylistically looking like a link with a large tap-target would be better. Where there are multiple buttons it might look weird and things like "Report this review" will be link-styled, not button-styled. So I think this should be "Edit my review" and should be an <a> tag instead with link styling.

Changing this to a <a> tag should be fine, I think the only adjustment you'd need would be a preventDefault() for the onClick...

kumar303 · 2017-09-25T15:53:25Z

@tofumatt thanks, it should be ready for another review. I gave the Edit anchor tag href="#" because I'm not sure how else to do that. I don't know if it's worth the effort of making a button style that looks like a link.

tofumatt

This is good but I just noticed some accessibility issues with the modal.

I'm of the opinion we should ditch modals but I wanted to bring them up:

When navigating with a keyboard, the modal gets activated but keyboard focus isn't changed to the textarea in the review modal. This means the user needs to manually tab over twenty times to get to the review modal. It might be nice to focus the textarea after "Edit my review" is activated.
Esc does not close the modal, at least on my Mac in Nightly.
Clicking to the sides of the modal doesn't close the modal. I need to click above or below it. I'm clicking constantly in this GIF:

But I think only the first is worth fixing in the scope of this review.

In the future I'd love to ditch modals.

So r+wc if we fix 1. in the above list 😄

tofumatt · 2017-09-25T23:31:47Z

src/amo/components/AddonReviewList/index.js

@@ -55,6 +52,8 @@ export class AddonReviewListBase extends React.Component {
  }

  loadDataIfNeeded(nextProps?: AddonReviewListProps) {
+    const lastAddon = this.props.addon;


Fair enough.

tofumatt · 2017-09-25T23:34:11Z

src/amo/components/AddonReviewList/index.js

@@ -204,7 +173,11 @@ export class AddonReviewListBase extends React.Component {
        <CardList>
          <ul>
            {allReviews.map((review, index) => {
-              return this.renderReview({ review, key: String(index) });
+              return (
+                <li className="AddonReviewList-li" key={String(index)}>


I'm not sure we use this class anywhere anymore (based on its removal from the stylesheet below) and it's a bit awkwardly named anyway. I'd be fine just removing it if it isn't used anywhere.

tofumatt · 2017-09-25T23:36:05Z

src/amo/components/AddonReviewListItem/styles.scss

+.AddonReviewListItem {
+  word-wrap: break-word;
+
+  > h3 {


I know these are from a previous patch but if it'd be possible to label this with a className and select them directly instead that'd be a bit less fragile. I've liked us not nesting selectors, especially direct selectors like this. I know it's way more verbose but it should help with HTML/CSS refactors.

If not though, it's okay.

kumar303 · 2017-09-26T17:16:23Z

I fixed the focus and looked into the other fixes but they weren't straight forward. I think on desktop we can do away with these modals. They were originally designed for mobile so clicking on the sides isn't really an issue (the gutter is pretty small). The modals might still be good UX for mobile screens.

I'll land this and then look at replacing the modal for desktop edits as part of mozilla/addons#10582

The failing tests will be resolved on master soon by #3244

tofumatt · 2017-09-26T17:17:49Z

I think on desktop we can do away with these modals. They were originally designed for mobile so clicking on the sides isn't really an issue (the gutter is pretty small). The modals might still be good UX for mobile screens.

All sounds good 👍 Thanks!

kumar303 added 15 commits September 19, 2017 17:00

Add stars to edit screen so editing can be done all at once

554f98e

Move AddonReview.js -> AddonReview/index.js

26d5166

Fix lint

fe305d3

Move AddonReviewList.js -> AddonReviewList/index.js

2d35bdb

Updated AddonReviewList tests to rely on redux mapped state better

dab2eef

Upgrade jest-enzyme to fix TypeError in expect() failure

9ac831f

Converted setAddonReviews to use an ID mapping, setReview still needs…

c150fc6

… work

throw errors when review.id is falsy

04dd432

Moved item rendering to AddonReviewListItem

e7ab10a

Got editing/hiding/re-editing from review list page all working

f16fa6e

remove TODO

8e7f321

Add comment about overlay

aac15ad

todo the todos

6456310

Merge branch 'master' into edit-review-iss3180

b8b8245

fix fallout after merge

a132eba

kumar303 commented Sep 22, 2017

View reviewed changes

kumar303 mentioned this pull request May 5, 2024

Fix review by user state so that it stores object IDs not objects mozilla/addons#10812

Closed

Link to bug in todo comment

61c7de4

kumar303 commented Sep 22, 2017

View reviewed changes

kumar303 added 2 commits September 22, 2017 17:11

update test spec

c5f63a6

Update yarn.lock after merging master

48395dc

kumar303 requested a review from tofumatt September 22, 2017 22:15

kumar303 changed the title ~~Allows users to edit their reviews~~ Allow users to edit their reviews Sep 22, 2017

tofumatt suggested changes Sep 23, 2017

View reviewed changes

kumar303 added 3 commits September 25, 2017 09:58

fixed translators comment

82246c9

fix css quotes

f3e6ee9

Use a link for edit action

6d2414d

kumar303 added 8 commits September 25, 2017 10:21

use dispatchClientMetadata not createStore

9c088f0

Updated some legacy assertions

b35c403

createStore -> dispatchClientMetadata

643d63a

swap createStore for dispatchClientMetadata

e80646d

fix nit

9ed9050

fix tests spec

c7e2d88

fix spec

dfe726e

Revert changes to the 'Submit review' text

1bcd6f0

kumar303 requested a review from tofumatt September 25, 2017 15:53

Merge branch 'master' into edit-review-iss3180

3e73cf0

tofumatt approved these changes Sep 25, 2017

View reviewed changes

kumar303 added 4 commits September 26, 2017 10:31

Merge branch 'master' into edit-review-iss3180

475aacc

Remove unused AddonReviewList-li class

d2f013a

Focus overlay textare on mount

a1f8b93

fix HTML global types, mostly for Flow

bf55d18

kumar303 merged commit 5a33c74 into mozilla:master Sep 26, 2017

kumar303 deleted the edit-review-iss3180 branch September 26, 2017 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to edit their reviews #3220

Allow users to edit their reviews #3220

kumar303 commented Sep 22, 2017 •

edited

kumar303 Sep 22, 2017

tofumatt Sep 23, 2017

kumar303 Sep 22, 2017 •

edited

tofumatt Sep 23, 2017

kumar303 Sep 25, 2017

kumar303 Sep 22, 2017

kumar303 Sep 22, 2017 •

edited

tofumatt Sep 23, 2017

codecov-io commented Sep 22, 2017 •

edited

tofumatt left a comment

tofumatt Sep 23, 2017

tofumatt Sep 23, 2017

tofumatt Sep 23, 2017

tofumatt Sep 23, 2017

tofumatt Sep 23, 2017

kumar303 Sep 25, 2017

tofumatt Sep 25, 2017

tofumatt Sep 23, 2017

tofumatt Sep 23, 2017

tofumatt Sep 23, 2017

tofumatt Sep 23, 2017

tofumatt Sep 23, 2017

kumar303 commented Sep 25, 2017

tofumatt left a comment

tofumatt Sep 25, 2017

tofumatt Sep 25, 2017

tofumatt Sep 25, 2017

kumar303 commented Sep 26, 2017

tofumatt commented Sep 26, 2017

Allow users to edit their reviews #3220

Allow users to edit their reviews #3220

Conversation

kumar303 commented Sep 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 Sep 22, 2017 • edited

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

kumar303 Sep 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 22, 2017 • edited

Codecov Report

tofumatt 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

kumar303 commented Sep 25, 2017

tofumatt 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

kumar303 commented Sep 26, 2017

tofumatt commented Sep 26, 2017

kumar303 commented Sep 22, 2017 •

edited

kumar303 Sep 22, 2017 •

edited

kumar303 Sep 22, 2017 •

edited

codecov-io commented Sep 22, 2017 •

edited