ShapeShift integration #652
No reviewers
Labels
No labels
accessibility
app-parity
area: creator
area: daemon
area: design
area: devops
area: discovery
area: docs
area: installer
area: internal
area: livestream
area: performance
area: proposal
area: reposts
area: rewards
area: search
area: security
area: subscriptions
area: sync
area: ux
area: viewer
area: wallet
BEAMER
channel
comments
community PR
consider soon
core team
css
dependencies
electron
Epic
feature request
first-timers-only
good first issue
hacktoberfest
help wanted
hub-dependent
icebox
Invalid
level: 0
level: 1
level: 2
level: 3
level: 4
merge when green
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
notifications
odysee
on hold
playlists
priority: blocker
priority: high
priority: low
priority: medium
protocol dependent
recsys
redesign
regression
resilience
sdk dependent
Tom's Wishlist
trending
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
windows
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: LBRYCommunity/lbry-desktop#652
Loading…
Reference in a new issue
No description provided.
Delete branch "shapeshift"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
For https://github.com/lbryio/lbry-app/issues/609
Looks like this
My feedback:
<p>text IMO.Looking at this now. By the way, you dont need to use your own forked repo for branches in github anymore. You can just branch off of this repo and push your branch to create a pull request.
I'll let you know my thoughts.
Some code feedback.
Shift states should probably be constants.
Should this be in the action?
Consider if
<BusyMessage>is appropriate hereGreat work starting a validation library! Let's determine a consistent signature for validation functions.
One that I've seen and like is that validators return sanitized values and except if they encounter an error.
So far, this is two inconsistent signatures, one that returns true/false, and one that returns an error set.
Let's make this data to be consistent.
I think these additions are likely not necessary. Just put on the components as relevant.
What do you think of allowing this wrapper to handle the
Object.assignthat every reducer does? Something like...In writing this, I noticed that the reducers you added in
shape_shift.jsdo not include theObject.assigncalls. Aren't those always necessary?Not if using the spread operator
....https://twitter.com/dan_abramov/status/688087202312491008
We could still add that to the
handleActionsfunction anyway. So we don't have to add...statein every reducer handlerObject.assignisn't a deep clone either.Object.assign({}, state, { foo: "bar"})and{ ...state, { foo: "bar" } }are functionally equivalent.(Correct me if above seems wrong to you.)
Yeah they are the same. I believe babel transpiles the spread operator into object.assign
I think it should stay here
Alright @kauffj do your worst. This code is starting to look a little wonky after looking it at so much so I'm sure there are a few things I missed/should fix. Working good though.
Source code only review.
@ -0,0 +4,4 @@export default ({ dark, className }) => {return (<divclassName={classnames(@IGassmann @liamcardenas please be aware of this pattern/design for determining class names that @seanyesmunt has added.
We have a
<SubmitButton>component used elsewhere. Please use that instead.(This code also has a styling error: the text weight when rendered as a button is less than that when rendered as an anchor.)
Can the existing
credit-amountclasses be used for this? I'm generally opposed to this type of classname-as-styling, though you can certainly find other examples in the code. Ifcredit-amountdoes not work, then I would instead propose new CSS classes specifically for styling financial values, notfont-bold.Unused class name.
Can this behavior be added to the existing
Addresscomponent? If not all<Address>components should have a button, then this can be an optional property.There is already code to detect external addresses on links and call
shell.OpenExternal. Just a simplehrefproperty is sufficient here.IMO this and all of the definitions below it probably shouldn't exist.
@ -98,4 +98,4 @@ $button-focus-shift: 12%;.button--submit {Isn't there already a disabled style? Can these share definitions?
Probably
$spacing-verticalfor consistency.Most of these paddings should be
$spacing-verticalor 1/2$spacing-vertical.Is a
shapeshift__errorsufficiently different from other errors that it needs it's own styling? Same for success.Does this actually require it's own styling? Is there no other styling that already exists that this should match?
This scares me.
$spacing-vertical* 3@ -0,0 +1,58 @@.spinner {👏 good refactor
Good idea
Don't need this class. The success class is just to add the green color on the text. There isn't any styles that only do that, maybe it should be a more generic className?
Yeah...
Can't think of better way to do it. Nothing bad could really happen, at least nothing worse than not having this. If someone changes the form content to be a different height, it will just "jump" to that height after it's rendered. The same thing would happen if we don't have this style, it will "jump" and increase the height when the form is rendered.
Happy to change it if we can think of a better way. I just hate having content jumping when stuff is loading
Not a fan of these? I think they can be pretty helpful to avoid creating a brand new class just to add a simple display style.
Totally down to get rid of them if you don't want them.
just curious, we just merged the react upgrade. I saw you changed all the React.PropTypes.etc... to just PropTypes.etc...
I believe this will break (there probably won't be a merge conflict because this is a new file. Maybe merge with master and re-check that this works after making necessary changes?
@ -0,0 +4,4 @@export default ({ dark, className }) => {return (<divclassName={classnames(Interesting, why was it done this way? (no need to respond here, I'll ask about it tomorrow)
I don't see a better way of doing this either. As you said, worst case scenario, it will just do what it otherwise would have done had you not put this in. Best case scenario, it looks great.
But yeah, absolute px values are never great for dimensions :P
please delete
Whoops. You're right. I'll update
all good now 👍
please save to package.json
whoops. added
@liamcardenas I think we should merge this separate from adding analytics just because the PR is so big. Then we can also do some testing in an RC. We can hold off on doing an actual release until we add the analytics.
@seanyesmunt you're right, I removed the dependency
More feedback. Though I'm encountering an error reaching the second phase of a shift, will share on Slack.
Nice touch.
This should probably be
<FormRow type="select">(plus other properties). You can add afieldPostfixor similar property for the "for LBC" part, if necessary.This should probably be
<FormRow type="text">.More generally, it's important to avoid using class names in a way that doesn't reflect what they are. This is not a Wunderbar.
I'm guessing what may have happened here is you don't like the default/current text input style. That's fine and good!
But in that case, the solution should be:
(@liamcardenas @IGassmann please read above as well)
If this error is instead passed into
<FormRow>, it will have the existing shared error styling. As-is, it's black.Should be in a
__wrapper.Missing
__(optional but recommend) should be first sentence of help text rather than in the label.
Missing address. You can point this at lbry.io/faq/shapeshift.
In general, to avoid necessity of updating code once article is generated, it's fine to put a URL in and then put the (suggested) slug in the ticket. If the support team doesn't like your suggestion, they can always let you know.
These display classes aren't necessary.
Confirming these should be removed for now since no part of this PR requires them anymore. I will bring up your point about the usefulness of these on an upcoming meeting, or encourage you to do so if I don't.
All of the CSS from here and below this can likely be done via existing card and form CSS classes and styles.
This CSS should be able to go.
I think you can use
credit-amount--indicatorinstead. If you dislike this, you can componentize the rendering of this.I'm okay shipping this as-is.
However, if we wanted to avoid it, I think there's a potential solution where we render the component before shapeshift is ready, but render it with a very low opacity or
visibility: hidden. In addition, either turn off interaction or render a blocking element on top of entire component. Then we can absolutely center a loading icon on top of this, and have it match the height precisely.Try
button="text"here.Add "."
I was thinking this would be a link to the FAQ Tom writes up
Add ".". Also, let's link to FAQ here again.
I'm not in love with "New" here - maybe just "Reset"? Though I don't like this either. As a (potentially weird) user, I have a desire for things to be back in their default state when "Done". But I don't necessary want to do a new one. I just want it back to where it was. Not sure on best name for this.
Also this should probably be primary, whatever the label is.
Yeah definitely not a fan of the current input styling. I was thinking this was fine for now since the PR is getting bigger and bigger. But might as well do it now and avoid more tech debt. I will update the generic
inputstyle to be more inline with the wunderbarI think this should stay as just classes with regular html elements. I can make my case more tomorrow, but my main idea is this:
Shared components are good for elements that can very a lot, but only with small differences. Buttons have many different possible styles (different background colors, borders, labels, etc). A lot of different variation, but the component is generally the same, a box with some text in the middle.
For this element (
select), there can be many different ways to present it. I think it definitely makes sense to use a shared components if there might be inline errors or something we want to keep the same across the app, but there won't be any errors on thisselectelement. I just want the styling with text on the left and right, which is very easy just by using regular html elements (and the same classes that theFormRowcomponent uses).If we start building out the shared components so much that they meet every possible use case, they can quickly become huge, un-maintainable files that can be terrible pain to update/change.
Would love to hear everyone else's thoughts. @liamcardenas @IGassmann
I'll keep the min-height on
shapeshift__maxdepositto avoid content jumping when we fetch new max limits after the user changes the origin coinI don't think it's necessary to use the
card-xxstyles for this. It doesn't have anything to do with thecardstyle. It's just a small bit of markup that needs some padding. I think ideally we could just use as-vertical-paddinghelper class or something similar. We can discuss this more along with thedisplay-xxhelper classes.👍
card__contentis the class I'd propose using instead.👍
@kauffj only thing left (I think) is adding a link to the FAQ on lbry.io, and flow.
@seanyesmunt no need to block app PRs on FAQ content. Just put the link in the code, get the ticket open (I know this happened), and you can forget about it.
@kauffj sounds good. What should the url be?
@seanyesmunt lbry.io/faq/shapeshift
https://github.com/lbryio/lbry-app/pull/652#discussion_r153944702
(also on the lbry.io ticket)
@ -0,0 +1,3 @@declare module 'bluebird' {There's got to be a better way to do this. Can these be added to the
.flowconfig?@ -20,3 +20,3 @@modules: [appPath, "node_modules"],extensions: [".js", ".jsx", ".css"]extensions: [".js", ".jsx", ".css", ".json"]},This is required because the shapeshift.io module has an import like
const package = require('../package')It causes the bundle to increase quite a bit. I will make a pr to that project to change it to
const package = require('../package.json')which allow use to remove this@kauffj @liamcardenas
I could probably spend another week just on cleaning up the flow stuff. It is integrated into the entire shapeshift flow and already caught a few issues. I think it is good for now, but could certainly be presented a little better. I will come back to this as we add flow to more files and determine how we want to structure it.
One more time :)
Should be "alt" for Cancel.
What happened to the idea of a "Sent" button that displays the shift code for tracking purposes?
@ -0,0 +75,4 @@return (<div>{shiftState === statuses.NO_DEPOSITS && (The exchange rate and fees should still be displayed at this step.
@ -0,0 +79,4 @@<div><p>Send up to{" "}<span className="credit-amount--bold">This whole element should be the same style IMO. I'd be fine with just
credit-amount--boldfor the whole thing.This needs to be cast as a boolean.
I still dislike both this rule and the rule below. Nothing about Shapeshift seems special enough that it needs it's own spacing rules vs. other cards.
@ -0,0 +13,4 @@}.shapeshift__tx-info {min-height: 55px;If this is to avoid jumping, I'd rather just put placeholder text like "Calculating rates..."
Yes we can display the actual rate. I'll add that
@ -0,0 +13,4 @@}.shapeshift__tx-info {min-height: 55px;We would still need some sort of min height on that unless the heights are the same. I would rather not since this check is usually very quick and you would only see that for a small time before it goes back to the info text.
I'm not sure how much I like that button. What if they click it before actually sending? We would need some new bit of UI to say "We will keep pinging shapeshit to see if you actually sent it" (obviously worded better).
We say "this will update automatically" and have a link to the shapeshift status page, I don't see any reason to create another shift status "user says they sent it but shapeshift hasn't received it"
@ -0,0 +1,3 @@declare module 'bluebird' {Have you checked if flow-typed has any of the type definitions?
https://github.com/flowtype/flow-typed
yarn run flow-typed install bluebirdoutside of the constructor, I think you need to add
continuousFetch: ?number;as per https://flow.org/en/docs/types/classes/
this should get rid of all the flow fix mes
@ -0,0 +20,4 @@// All ShapeShift actions// Action types defined in the reducer will contain some payloadexport type Action =| { type: types.GET_SUPPORTED_COINS_START }I get that you are defining these here so as not to create extra boilerplate work, but I wonder if all of the types should be defined in the same place? I think this is probably best, but I just wanted to throw this question out there in case anyone has any thoughts on it
@ -0,0 +37,4 @@// Basic thunk types// It would be nice to import these from types/common// Not sure how that would work since they rely on the Action typetype PromiseAction = Promise<Action>;can you make these generic and move them to a different folder as you alluded to in the comment? I also am not a huge fan of using the any type here but for now I think this is good.
i.e.
I don't want you to get bogged down with this if for some reason its really difficult
@ -34,6 +34,11 @@ bodycolor: var(--color-meta-light);}.credit-amount--boldis this used elsewhere in the app or is it just for shapeshift? I don't have a criticism here, I'm just wondering what we decided about using reusable vs case specific classes?
@ -0,0 +20,4 @@// All ShapeShift actions// Action types defined in the reducer will contain some payloadexport type Action =| { type: types.GET_SUPPORTED_COINS_START }My proposed design is:
typesfolder inside ofjs.typeswill eventually swallowconstants.Actiontype is defined inAction.jsinside of this folder.Actiontype explicitly enumerates all possible action type values.My biggest question about this design is whether there should only be one action type, or whether we should use subtypes for each possible action and payload signature.
@ -0,0 +20,4 @@// All ShapeShift actions// Action types defined in the reducer will contain some payloadexport type Action =| { type: types.GET_SUPPORTED_COINS_START }Possibly better to discuss this here https://github.com/lbryio/lbry-app/issues/810
@ -0,0 +20,4 @@// All ShapeShift actions// Action types defined in the reducer will contain some payloadexport type Action =| { type: types.GET_SUPPORTED_COINS_START }Let's not block this PR on getting action definition exactly correct.