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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include Add-on name and Add-on Icon in each review under "My reviews" on User Profile #12426

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

shribyte
Copy link

@shribyte shribyte commented Sep 25, 2023

Fixes mozilla/addons#2050

This patch includes the Add-on Name and Add-on Icon in each review under "My reviews", on the User Profile.

Before:
Screenshot 2023-09-25 at 11 03 43 AM

After:
Screenshot 2023-09-25 at 11 02 14 AM

…ser Profile

This change conditionally renders the add-on's name and icon when viewing a review on the User Profile page.

This reduces redundancy on the Add-on Reviews page, ensuring that the add-on name and icon are only displayed where it adds value and clarity.
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (0094a99) 98.50% compared to head (9d086d9) 98.50%.
Report is 20 commits behind head on master.

❗ Current head 9d086d9 differs from pull request most recent head adc8f36. Consider uploading reports for the commit adc8f36 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master   mozilla/addons-frontend#12426   +/-   ##
=======================================
  Coverage   98.50%   98.50%           
=======================================
  Files         258      258           
  Lines       10177    10189   +12     
  Branches     3060     3071   +11     
=======================================
+ Hits        10025    10037   +12     
  Misses        144      144           
  Partials        8        8           
Files Coverage Δ
src/amo/actions/reviews.js 100.00% <100.00%> (ø)
src/amo/api/reviews.js 100.00% <ø> (ø)
src/amo/components/AddonReviewCard/index.js 100.00% <100.00%> (ø)
src/amo/pages/UserProfile/index.js 100.00% <ø> (ø)
src/amo/reducers/reviews.js 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a UX mockup in the issue - https://github.com/mozilla/addons-frontend/issues/5858#issuecomment-417794798 - can we have it look like that?

src/amo/actions/reviews.js Outdated Show resolved Hide resolved
src/amo/components/AddonReviewCard/index.js Outdated Show resolved Hide resolved
src/amo/components/AddonReviewCard/styles.scss Outdated Show resolved Hide resolved
src/amo/components/AddonReviewCard/index.js Outdated Show resolved Hide resolved
@shribyte
Copy link
Author

There's a UX mockup in the issue - #5858 (comment) - can we have it look like that?

Sure, I will be pushing changes per the UX mockup tomorrow.

@shribyte shribyte requested a review from eviljeff September 27, 2023 16:20
@shribyte
Copy link
Author

shribyte commented Sep 27, 2023

Here's the UI after some styling changes, it's now similar to the UX mockup.

Screenshot 2023-09-27 at 12 21 19 PM

I've tried to avoid modifying the styling of existing elements (such as the rating and review body being pushed to the right, to afford more space for the icon), since that may have cascading effects across multiple views and would require re-styling a number of elements.

@eviljeff
Copy link
Member

We need a test in TestAddonReviewCard that checks this extra markup is displayed when isUserProfile is true.

@diox
Copy link
Member

diox commented Sep 29, 2023

I've tried to avoid modifying the styling of existing elements (such as the rating and review body being pushed to the right, to afford more space for the icon), since that may have cascading effects across multiple views and would require re-styling a number of elements.

It looks a little weird with the icon outside like that. IMHO the icon should be left-aligned with the stars and text on that page.

This commit enhances the localization support of the addon's name within reviews by ensuring the user's current locale is taken into consideration.
@shribyte
Copy link
Author

Here's a screenshot of a hard-coded icon to make it simpler to visualize the left-align:

Screenshot 2023-09-29 at 1 44 14 PM

): UserReviewType {
const effectiveLang = lang || 'en-US';
Copy link
Author

@shribyte shribyte Oct 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests in amo/reducers/test_reviews.js fail invariant checks without this line.

in analogous tests, calls to selectLocalizedContent are made by functions such as createInternalAddonWithLang that call createInternalAddon with lang set to DEFAULT_LANG_IN_TESTS.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need an equivalent function (e.g. createInternalReviewWithLang) that does a similar thing, and then change the calls in the tests in reducers/test_reviews.js to use that utility function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a lot of different approaches with createInternalReview changed to createInternalReviewWithLang. There are many different test files which use createInternalReview, and the number of invariant checks failing substantially increase.

Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some fixes with the tests

): UserReviewType {
const effectiveLang = lang || 'en-US';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need an equivalent function (e.g. createInternalReviewWithLang) that does a similar thing, and then change the calls in the tests in reducers/test_reviews.js to use that utility function.

@shribyte
Copy link
Author

shribyte commented Oct 2, 2023

there are some createInternalReview calls in amo/reducers/reviews.js that also make the invariant checks in selectLocalizedContent fail.
What should I do about these? They are invoked via reviewReducerfunction in reducers/test_reviews.js

As discussed with @eviljeff, this may be due to some of the existing tests not having the correct setup.

@shribyte
Copy link
Author

shribyte commented Oct 2, 2023

I assume that some of the existing tests don't have the correct setup. The fix should otherwise be ready to merge. I will move on to other bugs.

@eviljeff
Copy link
Member

eviljeff commented Oct 3, 2023

e3409a1 reduces the failing tests to 6.

@diox
Copy link
Member

diox commented Oct 11, 2023

@shribyte could you look at the last failing tests ?

@shribyte
Copy link
Author

shribyte commented Oct 11, 2023

@diox I looked at it some time back and couldn't exactly figure out why some existing tests were failing despite the state.lang additions.

Will look into it again after I'm done with my current assigned bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No add-on names associated with ratings when viewed from My Profile
3 participants