Instant Purchase #541

2017-09-08T07:12:39+02:00

alexliebowitz commented

2017-09-08 07:12:39 +02:00

(Migrated from github.com)

Solves #497.

👍 1 🎉 1

kauffj (Migrated from github.com) reviewed

2017-09-08 07:12:39 +02:00

alexliebowitz commented

2017-09-08 07:29:37 +02:00

(Migrated from github.com)

@kauffj

The entry in the settings page is mostly done. Still needs some state management fixes, and will need feedback on the UI as well:
screen shot 2017-09-08 at 1 31 30 am

Any thoughts/corrections on the wording?
Do we want the default currency to be LBC or dollars? Should dollars even be an option? We seem to prefer LBC for tiny amounts like this.
You said this and the max key fee should be grouped together in one section. If we do that we're going to need to figure out how we want it to look, and possibly add new styles to deal with the concept of sub-sections within tiles.

@kauffj The entry in the settings page is mostly done. Still needs some state management fixes, and will need feedback on the UI as well: <img width="683" alt="screen shot 2017-09-08 at 1 31 30 am" src="https://user-images.githubusercontent.com/7283989/30197410-9dcb2aa0-9435-11e7-9d00-e81b9c85564c.png"> <img width="683" alt="screen shot 2017-09-08 at 1 31 43 am" src="https://user-images.githubusercontent.com/7283989/30197419-a2fa4416-9435-11e7-9789-f091778facb6.png"> - Any thoughts/corrections on the wording? - Do we want the default currency to be LBC or dollars? Should dollars even be an option? We seem to prefer LBC for tiny amounts like this. - You said this and the max key fee should be grouped together in one section. If we do that we're going to need to figure out how we want it to look, and possibly add new styles to deal with the concept of sub-sections within tiles.

alexliebowitz commented

2017-09-11 02:43:10 +02:00

(Migrated from github.com)

@kauffj: Once we deal with the above UI issues and the LBC vs. USD issue, this is ready to go. I added an inline comment about the currency stuff.

alexliebowitz (Migrated from github.com) reviewed

2017-09-11 02:46:14 +02:00

ui/js/actions/content.js

					
				@ -341,0 +338,4 @@

				      // file manually on their file system, so we need to dispatch a

				      // doLoadVideo action to reconstruct the file from the blobs

				      if (!fileInfo.written_bytes) dispatch(doLoadVideo(uri));

If we want to make it possible to have the max instant purchase price be denominated in USD, then we'll have to do a currency conversion here. If not, then we'll have to add an option to FormFieldPrice to disable the currency selector and always use LBC (very easy).

kauffj (Migrated from github.com) reviewed

2017-09-14 21:25:08 +02:00

ui/js/actions/content.js

					
				@ -341,0 +338,4 @@

				      // file manually on their file system, so we need to dispatch a

				      // doLoadVideo action to reconstruct the file from the blobs

				      if (!fileInfo.written_bytes) dispatch(doLoadVideo(uri));

I believe the app already has the ability to do client side currency conversion, so it shouldn't be too bad.

kauffj (Migrated from github.com) reviewed

2017-09-14 21:25:33 +02:00

ui/js/actions/content.js

					
				@ -341,0 +338,4 @@

				      // file manually on their file system, so we need to dispatch a

				      // doLoadVideo action to reconstruct the file from the blobs

				      if (!fileInfo.written_bytes) dispatch(doLoadVideo(uri));

Also, make sure to put these in const/settings.js

Also, make sure to put these in `const/settings.js`

kauffj commented

2017-09-14 21:30:38 +02:00

(Migrated from github.com)

USD should be an option.
Yes, group them. The headings become the labels. The new heading is "Purchase Settings". No new styles are necessary, these are just two form rows with labels, the styles that already exist work fine.
Proposed radio choices:
- "Ask for confirmation of all purchases"
- "Single-click purchasing of content less than..."

- USD should be an option. - Yes, group them. The headings become the labels. The new heading is "Purchase Settings". No new styles are necessary, these are just two form rows with labels, the styles that already exist work fine. - Proposed radio choices: - "Ask for confirmation of all purchases" - "Single-click purchasing of content less than..."

alexliebowitz (Migrated from github.com) reviewed

2017-09-15 07:44:08 +02:00

ui/js/actions/content.js

					
				@ -341,0 +338,4 @@

				      // file manually on their file system, so we need to dispatch a

				      // doLoadVideo action to reconstruct the file from the blobs

				      if (!fileInfo.written_bytes) dispatch(doLoadVideo(uri));

Oh, right. OK, I'll make it handle USD. You'd think I would remember that we have client-side currency conversion, given that I wrote the code for it...

alexliebowitz commented

2017-09-17 04:18:34 +02:00

(Migrated from github.com)

So this creates a few more wording issues. The Max Purchase Price section will need to be reworded as well since the options don't make sense without the previous header:

Also, currently the max purchase price section has a totally separate wording when the option is not chosen ("Choose limit"). It sounds like you're saying the new section would have the same wording either way, but the "..." changes into a price field when you choose the option. I guess we would want to keep that consistent betweeen both.

Here's one possibility just as a starting point:

Purchase Settings

No maximum price when downloading content
Don't download content more than... / Don't download content more than [price field]

--

Ask for confirmation of all purchases
Single-click purchasing of content less than... / Single-click purchasing of content less than [price field]

An upshot of this wording is that neither of them would need helper text to make sense.

Also, we haven't discussed the order of the two sections. It could make sense to flip them, so it's minimum and then maximum.

So this creates a few more wording issues. The Max Purchase Price section will need to be reworded as well since the options don't make sense without the previous header: <img width="817" alt="screen shot 2017-09-16 at 10 03 08 pm" src="https://user-images.githubusercontent.com/7283989/30517369-e1b93bac-9b2a-11e7-9578-dcedcdca2269.png"> Also, currently the max purchase price section has a totally separate wording when the option is not chosen ("Choose limit"). It sounds like you're saying the new section would have the same wording either way, but the "..." changes into a price field when you choose the option. I guess we would want to keep that consistent betweeen both. Here's one possibility just as a starting point: **Purchase Settings** - No maximum price when downloading content - Don't download content more than... / Don't download content more than [price field] -- - Ask for confirmation of all purchases - Single-click purchasing of content less than... / Single-click purchasing of content less than [price field] An upshot of this wording is that neither of them would need helper text to make sense. Also, we haven't discussed the order of the two sections. It could make sense to flip them, so it's minimum and then maximum.

kauffj (Migrated from github.com) requested changes

2017-09-17 18:25:37 +02:00

kauffj (Migrated from github.com) left a comment

Some more feedback. I think there is some settings clean up in v16 as well. If you're working on this before v16 makes it into master, you may want to work on it there. Hoping to open PR for v16 into master today.

Some more feedback. I think there is some settings clean up in `v16` as well. If you're working on this before `v16` makes it into master, you may want to work on it there. Hoping to open PR for `v16` into master today.

ui/js/actions/content.js

					
				@ -343,0 +347,4 @@

				      return Promise.resolve();

				    }

				    const costInfo = makeSelectCostInfoForUri(uri)(state);

Settings should be read selector/settings.js.

Settings should be read `selector/settings.js`.

ui/js/actions/content.js

Should this be it's own conversion promise? Wonder if there is a clean way to do this without a promise.

ui/js/page/settings/view.jsx

					
				@ -14,2 +14,4 @@

				    this.state = {

				      instantPurchaseEnabled: props.instantPurchaseEnabled,

				      instantPurchaseMax: props.instantPurchaseMax,

Make constants.

ui/js/page/settings/view.jsx

These should probably be FormRow components, and definitely need labels.

These should probably be `FormRow` components, and definitely need labels.

alexliebowitz (Migrated from github.com) reviewed

2017-09-17 18:43:30 +02:00

ui/js/actions/content.js

It seems like there's some kind of caching going on in lbryio.getExchangeRates(), but even then, there's no guarantee that the exchange rates were refreshed recently. We could have it automatically update into the cache at intervals; then we could change lbryio.getExchangeRates() to be synchronous and directly return a result from the cache (and/or add a function like lbryio.convert() that does the same). Otherwise there's no guarantee that we won't have to go out to the server for this call, so there needs to be a promise or callback.

It seems like there's some kind of caching going on in `lbryio.getExchangeRates()`, but even then, there's no guarantee that the exchange rates were refreshed recently. We could have it automatically update into the cache at intervals; then we could change `lbryio.getExchangeRates()` to be synchronous and directly return a result from the cache (and/or add a function like `lbryio.convert()` that does the same). Otherwise there's no guarantee that we won't have to go out to the server for this call, so there needs to be a promise or callback.

alexliebowitz (Migrated from github.com) reviewed

2017-09-18 08:34:02 +02:00

ui/js/page/settings/view.jsx

I converted the "disabled" option to a FormRow. For the second option (where you choose the actual setting), I left it as a div for now. If we made it a FormRow, we would have to stuff the price selector inside the label prop, which feels awkward. The Max Key Fee section also uses a div, I'm guessing for the same reason. Let me know if you'd rather it be inside the label, it's very quick fix.

I added helper text in the most recent commit; let me know if you have any adjustments on the wording.

I converted the "disabled" option to a `FormRow`. For the second option (where you choose the actual setting), I left it as a div for now. If we made it a `FormRow`, we would have to stuff the price selector inside the label prop, which feels awkward. The Max Key Fee section also uses a div, I'm guessing for the same reason. Let me know if you'd rather it be inside the label, it's very quick fix. I added helper text in the most recent commit; let me know if you have any adjustments on the wording.

kauffj commented

2017-09-23 00:36:16 +02:00

(Migrated from github.com)

@alexliebowitz made very few additional changes to this, basically just adding labels. You can see here lbryio/lbry-app@b24b6e2e8e

Man do we need better form components!

@alexliebowitz made very few additional changes to this, basically just adding labels. You can see here https://github.com/lbryio/lbry-app/commit/b24b6e2e8e4b7ab7750f82776cb2f5cb641201be Man do we need better form components!

Sign in to join this conversation.

No reviewers

kauffj