Improve type checking for card verify #2057

2018-10-25T23:44:35+02:00

amelzer commented

2018-10-25 23:44:35 +02:00

(Migrated from github.com)

Improved type checking for CardVerify component.

neb-b (Migrated from github.com) requested changes 2018-10-26 04:39:35 +02:00

neb-b (Migrated from github.com) left a comment

Thanks for the PR! 🙂

We don't need the two classes you added (they aren't adding any types), but the other fixes you added are valid. Make sure to check out our contributing document.

Can we send you some LBC as appreciation?

Thanks for the PR! 🙂 We don't need the two classes you added (they aren't adding any types), but the other fixes you added are valid. Make sure to check out our [contributing document](https://github.com/lbryio/lbry-desktop/blob/master/CONTRIBUTING.md). Can we send you [some LBC as appreciation](https://lbry.io/faq/appreciation)?

src/renderer/component/cardVerify/view.jsx

					
				@ -10,0 +20,4 @@

				    close: Function,

				  };

				}

neb-b (Migrated from github.com) commented

2018-10-26 04:37:37 +02:00

We don't need these. They aren't doing anything as is. Also I belive the declare class syntax is wrong.

We don't need these. They aren't doing anything as is. Also I belive the `declare class` syntax is wrong.

amelzer (Migrated from github.com) reviewed 2018-10-26 09:47:39 +02:00

src/renderer/component/cardVerify/view.jsx

					
				@ -10,0 +20,4 @@

				    close: Function,

				  };

				}

amelzer (Migrated from github.com) commented

2018-10-26 09:47:39 +02:00

Thank you for your feedback.

The classes are types for the global CardVerify object that you are accessing after adding the script. Not declaring classes for them results in Flow errors

Cannot resolve name CardVerify.
Cannot resolve name StripeCheckout.

As this is a global variable, it would make sense in my opinion to also declare a global class like suggested here: https://flow.org/en/docs/libdefs/creation/#toc-declaring-a-global-class

The goal of my changes was to eliminate the lint & flow errors and therefore I added class definitions for the accessed global objects.

Thank you for your feedback. The classes are types for the global `CardVerify` object that you are accessing after adding the script. Not declaring classes for them results in Flow errors `Cannot resolve name CardVerify.` `Cannot resolve name StripeCheckout.` As this is a global variable, it would make sense in my opinion to also declare a global class like suggested here: https://flow.org/en/docs/libdefs/creation/#toc-declaring-a-global-class The goal of my changes was to eliminate the lint & flow errors and therefore I added class definitions for the accessed global objects.

neb-b (Migrated from github.com) reviewed 2018-10-26 17:50:21 +02:00

src/renderer/component/cardVerify/view.jsx

					
				@ -10,0 +20,4 @@

				    close: Function,

				  };

				}

neb-b (Migrated from github.com) commented

2018-10-26 17:50:21 +02:00

Ah you are right! I missed the part where we access CardVerify when I first looked this over. Thanks for that link too 🙂 I'll definitely read over that.

Ah you are right! I missed the part where we access `CardVerify` when I first looked this over. Thanks for that link too 🙂 I'll definitely read over that.

neb-b (Migrated from github.com) approved these changes 2018-10-26 17:50:33 +02:00

neb-b commented

2018-10-26 17:52:18 +02:00

(Migrated from github.com)

Thanks again for the PR! If you haven't yet, make sure to check out the contributing guide.

Can we send you some LBC as appreciation?

Thanks again for the PR! If you haven't yet, make sure to check out the [contributing guide](https://github.com/lbryio/lbry-desktop/blob/master/CONTRIBUTING.md). Can we [send you some LBC as appreciation](https://lbry.io/faq/appreciation)?

amelzer commented

2018-10-27 19:29:28 +02:00

(Migrated from github.com)

@seanyesmunt Not needed, thank you :)

tzarebczan commented

2018-10-30 18:08:13 +01:00

(Migrated from github.com)

@amelzer how about an exclusive LBRY Hacktoberfest t-shirt? Shoot us an email at hello@lbry.io so we can get the form over to you :)

@amelzer how about an [exclusive LBRY Hacktoberfest t-shirt](https://twitter.com/LBRYio/status/1056910558006599681)? Shoot us an email at hello@lbry.io so we can get the form over to you :)

neb-b commented

2018-10-30 18:40:16 +01:00

(Migrated from github.com)

@amelzer I reverted your PR. I guess I didn't look close enough but upon testing Tom noticed it was breaking the app. I can now see it was because of your variable name changes.

CardVerify.stripeHandler is supposed to reference the component, not the type. We could change that to this.stripeHandler.

@amelzer I reverted your PR. I guess I didn't look close enough but upon testing Tom noticed it was breaking the app. I can now see it was because of your variable name changes. `CardVerify.stripeHandler` is supposed to reference the component, not the type. We could change that to `this.stripeHandler`.

amelzer commented

2018-10-31 15:01:14 +01:00

(Migrated from github.com)

@seanyesmunt Thanks for letting me know. I will create a new PR

Sign in to join this conversation.

No reviewers

neb-b