Calculate CI with distributions when scrolled into view #16
Conversation
…c stats. Moved stat parsing to it's own function
…kip products that have already been handled.
musicin3d
force-pushed
the
calc-onscroll
branch
from
ca0293a
to
cba8071
Compare
musicin3d
changed the title
|
This is pretty cool! I like the functionality |
|
I reorganized inject.js, so it with conflict with Chris's pull request... #19 |
The changes I made to |
# Conflicts: # src/chrome/inject.js
… process of calculating proportion before evaluating. Minor doc fixes. Corrected order of ratings in distribution
|
Resolved conflicts and fixed bugs. This is ready for final review. Tagging: #9 |
|
I don't know if this is a regression or a new bug. At https://www.amazon.com/s?me=A3EPIN8KW1GGQX&marketplaceID=ATVPDKIKX0DER the scraping fails almost entirely. Edit: It's a regression. I'll fix it. |
|
Sorry about delay in reviewing this, should be able to check it out this evening |
| run() | ||
| } | ||
| function run(){ | ||
| //TODO sometimes products are featured together, and this selects the entire block instead of each product |
There was a problem hiding this comment.
I noticed it also doesn't work when you're searching within a category, eg: https://www.amazon.ca/s?k=notes&i=alexa-skills&ref=nb_sb_noss
There was a problem hiding this comment.
Looks like this is the same issue you pointed out earlier
| const leftover = [] | ||
| for(let product of noDistProducts) { | ||
| if (productInView(product)){ | ||
| // noinspection JSIgnoredPromiseFromCall |
There was a problem hiding this comment.
shouldn't you also check for product.reviews? I noticed that in products without reviews, the avg viz doesn't get added, but a blank dist viz does
There was a problem hiding this comment.
Good find! I've pushed an update that filters out those products before setting noDistProducts
|
I'll try to resolve all issues tomorrow night. |
|
Closing due to the revelation in #10 (comment) |
|
@musicin3d you don’t think it still might be worth taking this approach even if we do use the average star rating? Especially if we want to offer a mode where the visualization is injected into the popover? |
|
Inserting things into the DOM isn't very expensive. In fact, it's probably
be better to do it all at once whenever possible. If the calculations are
cheap as well, then I see no point in using the scroll event. My motivation
behind going down this path was that retrieving the individual ratings
required network requests. Those are expensive, and they were even being
throttled in this case.
If we're expecting some expensive calculations, then there would still be
some value in this. If we are not then I believe we can find a better way.
For example, we can simply insert a score under each rating, and we can
display our own popover when you mouse over that.
If we don't like that then we might be able to find a way to modify
Amazon's popover when it's created, but that could get messy regardless of
using the scroll event or not.
To get back out of the weeds... Are we expecting expensive calculations?
…On Thu, Apr 23, 2020, 11:17 PM Christopher Bryant ***@***.***> wrote:
@musicin3d <https://github.com/musicin3d> you don’t think it still might
be worth taking this approach even if we do use the average star rating?
Especially if we want to offer a mode where the visualization is injected
into the popover?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAICQTKOAGPQIDQZPHMNXD3ROEHG7ANCNFSM4LVOWJJA>
.
|
|
I see, thanks for clarifying. Yeah I don’t imagine we’d be doing any expensive calculations |
|
Hey @musicin3d, the changes you made in refactoring the scraping are still interesting even if didn't end up benefitting from using the distributions. So I think there's value in merging some of the commits in this PR to master 😛 |
|
Hey! I appreciate that. 😄 I'll go back through and make a new pull request
with the refactoring.
…On Mon, Apr 27, 2020, 10:24 PM Aecio ***@***.***> wrote:
Hey @musicin3d <https://github.com/musicin3d>, the changes you made in
refactoring the scraping are still interesting even if didn't end up
benefitting from using the distributions. So I think there's value in
merging some of the commits in this PR to master 😛
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAICQTJX575BLYHNNU7CLF3ROZD7DANCNFSM4LVOWJJA>
.
|
At the moment, this is just a proof of concept. It loads distributions, calculates the CI, and dumps the information to the page. It also highlights the product tile when it's finished.
There are a few bugs that still need to be worked out.