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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

I2664 - When pre-loading reviews, create the respective amount placeholders #7056

Merged
merged 8 commits into from Nov 27, 2018

Conversation

Projects
None yet
3 participants
@SeanPrashad
Copy link
Contributor

commented Nov 26, 2018

Fixes #2664: When pre-loading reviews, create only the number of reviews that the page will load

This patch will now render the respective amount of LoadingText, ie. placeholders, when a user navigates to an addons reviews page, replacing the current implementation of 4 placeholders.

The new criterion is as follows:

  1. If the addon has between 1 and 25 reviews, render the exact amount (ex. 3 LoadingText placeholders for 3 reviews):

    馃帴 Between 1 and 25 reviews (inclusive)

    loadingtext_pagination_exact_limit

  2. If the addon has more than 25 reviews, render 25 LoadingText placeholders (as per the pagination limit):

    馃帴 25+ reviews

    loadingtext_pagination_max_limit

  3. If the user navigates directly to the addon, bypassing the amount of reviews, render 4 LoadingText placeholders.

@codecov-io

This comment has been minimized.

Copy link

commented Nov 26, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #7056      +/-   ##
=========================================
+ Coverage    97.9%   97.9%   +<.01%     
=========================================
  Files         253     253              
  Lines        6905    6908       +3     
  Branches     1274    1276       +2     
=========================================
+ Hits         6760    6763       +3     
  Misses        131     131              
  Partials       14      14
Impacted Files Coverage 螖
src/amo/pages/AddonReviewList/index.js 100% <100%> (酶) 猬嗭笍

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 65af9c5...3247d63. Read the comment docs.

@willdurand willdurand self-assigned this Nov 26, 2018

@willdurand
Copy link
Member

left a comment

Looks super cool, thanks! I've questions and some change requests.

Show resolved Hide resolved src/amo/pages/AddonReviewList/index.js Outdated
Show resolved Hide resolved src/amo/pages/AddonReviewList/index.js Outdated
Show resolved Hide resolved tests/unit/amo/pages/TestAddonReviewList.js Outdated
Show resolved Hide resolved tests/unit/amo/pages/TestAddonReviewList.js Outdated

SeanPrashad added some commits Nov 25, 2018

Preload the respective number of reviews
To prevent a jarring user experience, this patch will allow a 1:1 ratio
of reviews to loading placeholders.

If the number of reviews to load exceeds 25, only show the first 25 as
per the pagination amount.

If the amount of reviews is undetermined, ie. loading the page directly,
generate a random amount of placeholders within 1 to 25 inclusive.

@SeanPrashad SeanPrashad force-pushed the SeanPrashad:issue-2664 branch from 3610bf7 to 6ede9e1 Nov 26, 2018

@SeanPrashad SeanPrashad force-pushed the SeanPrashad:issue-2664 branch from 6ede9e1 to 73e5861 Nov 26, 2018

SeanPrashad added some commits Nov 27, 2018

Update new tests for placeholders
1. Ensure that a 1:1 ratio of reviews to placeholders is rendered
2. Ensure only the DEFAULT_API_PAGE_SIZE amount os placeholders is
   rendered when the amount of reveiws exceeds it.
@SeanPrashad

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

We have a green from Travis!

I tested locally without my patch and things blow up (as expected since it will always load 4 placeholders):

without_my_patch

Please let me know if I can polish this up any further!! 馃樅

Show resolved Hide resolved tests/unit/amo/pages/TestAddonReviewList.js Outdated
Show resolved Hide resolved tests/unit/amo/pages/TestAddonReviewList.js Outdated
Show resolved Hide resolved tests/unit/amo/pages/TestAddonReviewList.js Outdated
Show resolved Hide resolved tests/unit/amo/pages/TestAddonReviewList.js Outdated
@SeanPrashad

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

I've updated the test descriptions and addressed the other nits - feel free to take a look when you have time @willdurand!

@willdurand willdurand merged commit 89f609f into mozilla:master Nov 27, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (Add-ons Team) No new, high severity issues
Details
@willdurand

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

Thanks @SeanPrashad!

@SeanPrashad SeanPrashad deleted the SeanPrashad:issue-2664 branch Nov 27, 2018

@SeanPrashad SeanPrashad changed the title I2664 - When pre-loading reviews, create the respective amount placeholders for reviews I2664 - When pre-loading reviews, create the respective amount placeholders Dec 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.