Convert AddonReviewList to a saga #2633
Conversation
kumar303
force-pushed
the
review-list-saga-iss2608
branch
from
440a5f1 to
4c54f30
Compare
There was a problem hiding this comment.
🎉
This is awesome. Tested it locally and the UX of going from search to add-on to reviews is great. Once we get search hooked up to redux it's gonna feel so natural navigating between pages.
All of my comments are little improvements (most are about the toHaveProps matcher) or nitpicks/minor things. Overall this looks great, super stoked. Thanks so much for all the work porting to sagas.
r+wc
| module.exports = { | ||
| apiHost: 'https://localhost', | ||
| allowErrorSimulation: true, | ||
| enableClientConsole: true, |
There was a problem hiding this comment.
Will this make test output (any) noiser? I imagine the aim of setting this to false here was to make tests not output a log of logging stuff.
There was a problem hiding this comment.
the aim of setting this to false here was to make tests not output a log of logging stuff.
I sure hope not because that would be the most cruel trick ever played on a developer. It's 100% essential to see logging in test output. However, I would love to hide the test output for passing tests. Maybe that's possible. As for reducing noise, you can still use --silent.
| const { i18n } = this.props; | ||
| const timestamp = i18n.moment(review.created).fromNow(); | ||
|
|
||
| let byLine; |
There was a problem hiding this comment.
I feel like byLine is a bit of a niche word... I know that's what this is but reviewAuthor would work equally well for what this is and might be a bit easier to parse.
There was a problem hiding this comment.
Byline has been a standard word since the 1950s. Naming it reviewAuthor is awkward because the code already uses a variable named review.userName. Plus, this text contains name and date which makes it more of a byline than just an author name.
| throw new Error('params.addonSlug cannot be falsey'); | ||
| } | ||
| if (!reviews || !addon) { | ||
| // TODO: add a spinner |
There was a problem hiding this comment.
Hey, we finally did it! 😄
| // TODO: add a spinner | ||
| return <div>{i18n.gettext('Loading...')}</div>; | ||
|
|
||
| // When reviews have not loaded yet, make a list of 4 empty reviews |
src/amo/sagas/reviews.js
Outdated
| yield put(showLoading()); | ||
| const api = yield select(getApi); | ||
| const response = yield call(getReviews, { | ||
| // Hide star ratings (reviews that do not have a body). |
There was a problem hiding this comment.
This could be clarified slightly: "Hide star-only ratings (reviews that do not have a body)".
All ratings have stars, so "star-only" might help the reader grok this a bit faster.
| expect(dispatch.firstCall.args[0]).toEqual(expectedAction); | ||
| }); | ||
| const root = render({ location: { query: { page: 3 } } }); | ||
| expect(root.find(Paginate).prop('currentPage')).toEqual(3); |
There was a problem hiding this comment.
toHaveProp matcher here too.
tests/unit/amo/sagas/testReviews.js
Outdated
| await sagaTester.waitFor(SET_ADDON_REVIEWS); | ||
| mockAmoApi.verify(); | ||
|
|
||
| expect(sagaTester.getCalledActions()[1]).toEqual(showLoading()); |
There was a problem hiding this comment.
In the categories test we did:
const calledActions = sagaTester.getCalledActions();
expect(calledActions[0]).toEqual(actions.categoriesFetch());
[...]I found it a touch prettier.
| expect(ratings[1].props.rating).toEqual(2); | ||
| expect(ratings[1].props.readOnly).toEqual(true); | ||
| expect(ratings.at(0).prop('rating')).toEqual(1); | ||
| expect(ratings.at(0).prop('readOnly')).toEqual(true); |
| expect(ratings[1].props.readOnly).toEqual(true); | ||
| expect(ratings.at(0).prop('rating')).toEqual(1); | ||
| expect(ratings.at(0).prop('readOnly')).toEqual(true); | ||
| expect(ratings.at(1).prop('rating')).toEqual(2); |
| expect(ratings.at(0).prop('rating')).toEqual(1); | ||
| expect(ratings.at(0).prop('readOnly')).toEqual(true); | ||
| expect(ratings.at(1).prop('rating')).toEqual(2); | ||
| expect(ratings.at(1).prop('readOnly')).toEqual(true); |
|
Oh, just one thing I noticed: if you load the reviews page from the server seems like we don't dispatch the
It will be good to start dispatching those largely based on URL. |
Good catch, fixed. Yeah, once we connect this to the URL then it won't require so much manual dispatch logic. |
|
Thanks for the reviews! |
Fixes mozilla/addons#10494