Skip to content

Conversation

kumar303
Copy link
Contributor

@kumar303 kumar303 commented Jun 30, 2017

Fixes mozilla/addons#10504

There are two main problems I'm fixing:

  • when a wrapped component gets updated with a new addon prop, the HOC wasn't querying the addon manager for data
  • when a wrapped component is updated, the HOC was defined in a way where it did not have access to the new properties

@kumar303
Copy link
Contributor Author

@tofumatt I tried not to change too much even though this code is a bit gnarly. This patch fixed the issue for me.

@kumar303 kumar303 requested a review from tofumatt June 30, 2017 21:58
@kumar303
Copy link
Contributor Author

Aw crap, this only fixed showing the install button. The install flow is still broken.

@kumar303 kumar303 removed the request for review from tofumatt June 30, 2017 22:04
@tofumatt
Copy link
Contributor

tofumatt commented Jun 30, 2017 via email

@kumar303 kumar303 requested a review from tofumatt July 1, 2017 01:52
@kumar303
Copy link
Contributor Author

kumar303 commented Jul 1, 2017

It should be ready for review now.

@kumar303
Copy link
Contributor Author

kumar303 commented Jul 1, 2017

The most helpful thing would be to test this out locally and make sure it's working -- we need to get it on dev ASAP. Tuesday is a US holiday.

@muffinresearch could you help test the patch too?

@kumar303 kumar303 requested a review from muffinresearch July 1, 2017 17:18
@tofumatt
Copy link
Contributor

tofumatt commented Jul 1, 2017

My flights were a bit delayed but I'm settled and such back home. I should be able to test tonight or tomorrow at the latest 👍

@tofumatt
Copy link
Contributor

tofumatt commented Jul 1, 2017

Just tested it quickly locally for extensions and themes–thus far seems all good. I'll have a look through the code ASAP, thanks for the quick patch.

Copy link
Contributor

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

I ran this locally and it worked. As you mentioned, it seems like largely exposing these props to the actual wrapped components and updating them correctly which wasn't happening before.

Thanks for doing some test clean up as well. This works for me, let's get it on -dev so we can get QA to test it.

Seeing your fix I'm sorry I didn't figure that out. Thanks again!

@muffinresearch
Copy link
Contributor

Just fixed the conflicts so I can try this out...

@kumar303 kumar303 force-pushed the fix-install-iss2636 branch from 1ba6176 to 8086cb3 Compare July 3, 2017 16:44
@muffinresearch
Copy link
Contributor

Install looks to work running charles to fake a.m.o for both the amo and discovery apps.

@kumar303 kumar303 merged commit 9b54c13 into mozilla:master Jul 3, 2017
@kumar303 kumar303 deleted the fix-install-iss2636 branch July 3, 2017 18:10
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.

First time when opening an add-on details page, the install radio button is greyed out
3 participants