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

RE: Add related categories to "More Info" section on add-on details #10761

Merged
merged 18 commits into from Aug 10, 2021

Conversation

xlisachan
Copy link
Contributor

@xlisachan xlisachan commented Jul 13, 2021

Fixes #2758

I am still updating TestAddonMoreInfo.js, but wanted to provide an update.

I took a look at #9253 as it was referenced as a starting point in the issue. However, as the addon's categories are already included in the addon props, it made more sense to start from scratch since there were changes made that did not seem necessary (e.g, updating the Addon page, Categories component and reducer).

This pull request includes the following updates:

  • Added 'Related Categories', updated the layout, alphabetized imports, etc (amo/components/AddonMoreInfo/index.js)
  • Added an AddonMoreInfo stylesheet (amo/components/AddonMoreInfo/styles.scss)
  • Created getRelatedCategories to help return the categories - I was not sure where to place this though, please let me know if it should go elsewhere. (amo/utils/addons.js)
  • Updated tests/unit/amo/utils/test_addons.js

Screenshots
Before
Screen Shot 2021-07-12 at 9 39 13 PM

Mock-up

After
Screen width: 425px
Screen Shot 2021-07-12 at 8 42 11 PM

Screen width: 768px
Screen Shot 2021-07-12 at 9 35 00 PM

Screen width: 1024px
Screen Shot 2021-07-12 at 8 42 56 PM

I tried to stay as close to the mock-up as possible, but I came across an example (see below) that became distorted especially between large (min-width: 720px) and extraLarge(min-width: 900px). I am not sure if it's just dummy text on dev, but I updated display to block for that width range.
Screen Shot 2021-07-12 at 8 43 51 PM

While the issue is to add 'Related Categories' to this panel, should I update the links to not be underlined as well?

As mentioned I am still working on updating TestAddonMoreInfo and will push it through when ready (or I'll let you know if I have any questions). Please let me know if you have any feedback when you have a moment. Thank you!!

@bobsilverberg bobsilverberg requested review from a team and willdurand and removed request for a team and willdurand July 13, 2021 12:15
@bobsilverberg
Copy link
Contributor

I have removed the review request for this PR as it requires some changes which I discussed with Lisa in our bi-weekly meeting.

@codecov
Copy link

codecov bot commented Jul 27, 2021

Codecov Report

Merging #10761 (6aaf7ad) into master (9f5f378) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10761   +/-   ##
=======================================
  Coverage   98.45%   98.45%           
=======================================
  Files         255      255           
  Lines        7448     7462   +14     
  Branches     1354     1357    +3     
=======================================
+ Hits         7333     7347   +14     
  Misses        107      107           
  Partials        8        8           
Impacted Files Coverage Δ
src/amo/components/AddonMoreInfo/index.js 100.00% <100.00%> (ø)

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 9f5f378...6aaf7ad. Read the comment docs.

@xlisachan
Copy link
Contributor Author

Thanks for the additional insight, @bobsilverberg !

I have pushed through two commits - (1) using #2758 as a starting point and adding the respective tests, (2) cleaning up the order of the imports in the AddonMoreInfo component.

I am working on updating the category names to display as links - I'll include screenshots when complete!

@xlisachan
Copy link
Contributor Author

xlisachan commented Jul 27, 2021

The latest push reflects updating the related categories' names to links (and test updates).

Screenshots
Before
Screen Shot 2021-07-27 at 1 07 24 PM

After
Screen Shot 2021-07-27 at 1 06 05 PM

Please review when you have a moment. Thank you!

@xlisachan xlisachan marked this pull request as ready for review July 27, 2021 17:16
@bobsilverberg bobsilverberg requested review from a team and eviljeff and removed request for a team July 27, 2021 17:24
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.

I was going to mention using css for joining the list with commas but Will beat me to it 😉

Looks like fine to me - I'm assuming @bobsilverberg has already been looking at it while it was a draft and he's okay with the general strategy

src/amo/reducers/categories.js Outdated Show resolved Hide resolved
tests/unit/amo/reducers/test_categories.js Outdated Show resolved Hide resolved
@bobsilverberg
Copy link
Contributor

I was going to mention using css for joining the list with commas but Will beat me to it 😉

Looks like fine to me - I'm assuming @bobsilverberg has already been looking at it while it was a draft and he's okay with the general strategy

I have not reviewed this PR at all, I just put it into the rotation, so don't assume anything! It looks like @willdurand is also looking at it, so I probably don't need to add my 2¢ as well, but I will take a look.

@xlisachan
Copy link
Contributor Author

Thanks for the feedback, @eviljeff and @willdurand ! The latest revision includes using the CSS approach to join the related categories as advised.

Regarding the Object.values(...) conversation, please see above comment.

Please review when you have a moment. Thanks!

@willdurand willdurand self-requested a review July 29, 2021 06:43
@eviljeff
Copy link
Member

Now #10798 is merged this is going to need a rebase. Can you adjust the way you build the list of categories so it does it in a similar way too?

There may be scope for abstracting how the <li> is built to a separate function that just takes a list of (url, anchor text) but it may also end up being more code than just repeating it to build the data structures from the quite different source (categories being a lot more complicated with localised names; tags just being simple text strings).

@xlisachan
Copy link
Contributor Author

xlisachan commented Jul 29, 2021

The latest push reflects adjusting how the list of categories are built. Please review when you have a moment. Thank you!

Screen Shot 2021-07-29 at 7 00 56 PM

@xlisachan xlisachan requested a review from eviljeff July 29, 2021 23:02
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.

looks good! Just the css change.
r+wc

src/amo/pages/AddonInfo/styles.scss Outdated Show resolved Hide resolved
@willdurand willdurand self-requested a review August 2, 2021 10:25
@xlisachan
Copy link
Contributor Author

Thanks for the feedback, @willdurand and @eviljeff !

The latest push removes the unnecessary errorHandler prop and updates the style from commas to small bullets - see screenshot below.

Screen Shot 2021-08-02 at 9 03 29 AM

Please review when you have a moment. Thanks!

@eviljeff
Copy link
Member

eviljeff commented Aug 2, 2021

(I have nothing more to add - it was a r+wc - don't know about @willdurand )

@willdurand
Copy link
Member

(I have nothing more to add - it was a r+wc - don't know about @willdurand )

Yeah, I'll do another review later today. I need to remember why we need a new error handler in this component and why it isn't a "fixed" handler.

Copy link
Member

@willdurand willdurand left a comment

Choose a reason for hiding this comment

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

Thanks, this is looking good but there is some more work needed to handle edge cases/errors.

src/amo/components/AddonMoreInfo/index.js Outdated Show resolved Hide resolved
src/amo/components/AddonMoreInfo/index.js Outdated Show resolved Hide resolved
src/amo/components/AddonMoreInfo/index.js Show resolved Hide resolved
src/amo/components/AddonMoreInfo/index.js Outdated Show resolved Hide resolved
src/amo/components/AddonMoreInfo/index.js Outdated Show resolved Hide resolved
src/amo/components/AddonMoreInfo/index.js Show resolved Hide resolved
tests/unit/amo/components/TestAddonMoreInfo.js Outdated Show resolved Hide resolved
@xlisachan
Copy link
Contributor Author

Thanks for the additional feedback, @willdurand !

The latest push includes updating variable names (clientApp, categories) and adding tests.

As mentioned in the conversations above, please let me know how I should proceed with the order of the information in this component as well as if there are additional tests that I should add.

Please review when you have a moment. Thanks!

Copy link
Member

@willdurand willdurand left a comment

Choose a reason for hiding this comment

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

This is looking good and it works well, thanks. I have some comments on the test part.

src/amo/components/AddonMoreInfo/index.js Show resolved Hide resolved
src/amo/components/AddonMoreInfo/index.js Outdated Show resolved Hide resolved
tests/unit/amo/components/TestAddonMoreInfo.js Outdated Show resolved Hide resolved
tests/unit/amo/components/TestAddonMoreInfo.js Outdated Show resolved Hide resolved
tests/unit/amo/components/TestAddonMoreInfo.js Outdated Show resolved Hide resolved
tests/unit/amo/components/TestAddonMoreInfo.js Outdated Show resolved Hide resolved
tests/unit/amo/components/TestAddonMoreInfo.js Outdated Show resolved Hide resolved
tests/unit/amo/components/TestAddonMoreInfo.js Outdated Show resolved Hide resolved
@xlisachan
Copy link
Contributor Author

Thanks for the feedback, @willdurand! The latest push reverts the order of information, adds the blank line, and includes updates for the tests.

I added a comment to an above conversation regarding the test cases. Please review when you have a moment. Thank you!

tests/unit/amo/components/TestAddonMoreInfo.js Outdated Show resolved Hide resolved
tests/unit/amo/components/TestAddonMoreInfo.js Outdated Show resolved Hide resolved
tests/unit/amo/components/TestAddonMoreInfo.js Outdated Show resolved Hide resolved
tests/unit/amo/components/TestAddonMoreInfo.js Outdated Show resolved Hide resolved
Copy link
Member

@willdurand willdurand left a comment

Choose a reason for hiding this comment

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

Thanks, this is looking very good. I made some suggestions, which I am going to accept myself so that we can land your patch 🎉

tests/unit/amo/components/TestAddonMoreInfo.js Outdated Show resolved Hide resolved
tests/unit/amo/components/TestAddonMoreInfo.js Outdated Show resolved Hide resolved
@willdurand
Copy link
Member

I added one more test case to cover the case where the add-on type does not have a category.

@willdurand willdurand merged commit 55dab59 into mozilla:master Aug 10, 2021
@xlisachan xlisachan deleted the addon_moreinfo branch August 10, 2021 12:53
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.

Add related categories to "More Info" section on add-on details
4 participants