Account popup #135
Account popup #135
Conversation
src/components/accounts/popup.js
Outdated
| const AccountPopup = connect( | ||
| (state, ownProps) => { | ||
| const account = state.get('accountPopup', Immutable.fromJS()); | ||
| const open = account === {}; |
There was a problem hiding this comment.
It looks like you are fetching accountPopup from the global state, which isn't necessary. Since the ADD ETC button is in the account page, you can just pass the account in as a prop. eg:
<AccountPopup account={ownProps.account} />
(assuming that is what you are trying to do)
There was a problem hiding this comment.
oh, looks like you are already doing that. You can also store popup state in a local state:
class AccountPopup extends React.Component {
constructor(props) {
super(props);
this.state = {
popupstuff
};
}
render() {
<Dialog />
}
} There was a problem hiding this comment.
I see -- that's similar to how the docs had it, too. Was a little concerned about tangling this.state with Redux's state...
I was aiming to make a button/dialogue that would be reusable for an account's page (accounts/show.js) as well as from an account list item (accounts/account.js), without having to load the dialog code more than once.
There was a problem hiding this comment.
oh, you mean just reuse the rendered component? like the ones in lib/elements/*.js?
There was a problem hiding this comment.
oooo, I hadn't looked at elements... that looks along the lines of what I was thinking.
Realizing now maybe I don't need to worry so much about multiple-rendered DOM elements (ie <AccountPopup/> because of the whole Virtual DOM thing. It may not be any simpler/elegant anyways, and would avoid a global
hm, will do some more homework this eve
|
FYI, in |
- remove popupstate from global - place popup in accounts list, show - unify button and dialog elements into 'popup' directive
whilei
force-pushed
the
account-popup-wip
branch
from
c7e43c3
to
973c493
Compare
Problem: they weren't having any impact... Follow up problem: get table columns to respond to width , especially so you can see the whole address
Doesn't work either... hm.
solution: use directive 'secondary' to make them themed red
solution: send to parent address instead
solution: add green style
solution: make gray with white color and change 'ADD ETHER' -> 'ADD ETC'
|
Please check my "Minimal amount" for transfer on the popup... I just used new Wei(21000000000); // Rough estimate tx gasprice; 21000 * 10^6not sure if that's what it should be. |
There was a problem hiding this comment.
Looks good ... the one part where you have inline comments might cause an error, because react doesn't know to interpret javascript comments unless they're in { ...}
Also my personal preference is to keep the ADD ETC button separate from the Dialog component, since the button is technically a part of the layout in which it appears, but no big deal 👍
src/components/accounts/popup.js
Outdated
| style={styles.openButton} /> | ||
| <Dialog | ||
| // title="Add Ether" | ||
| // actions={actions} |
There was a problem hiding this comment.
doesn't this cause an error?
There was a problem hiding this comment.
i have no idea why i got away with that
src/components/accounts/popup.js
Outdated
| const accounts = state.accounts.get('accounts'); | ||
| const pos = accounts.findKey((acc) => acc.get('id') === ownProps.account.get('id')); | ||
| const rates = state.accounts.get('rates'); | ||
| const gasPrice = new Wei(21000000000); // Rough estimate tx gasprice; 21000 * 10^6 |
There was a problem hiding this comment.
current gasprice about 21,338 MWei (http://gastracker.io/)
src/lib/types.js
Outdated
| if (typeof (r) === 'undefined' || r === null) { | ||
| r = 0; | ||
| } | ||
| if (typeof decimals === 'undefined' || decimals === null) { | ||
| decimals = 5; |
There was a problem hiding this comment.
maybe make default decimals 2 for fiat
|
And duly noted on preferable component separation... I would do except I'm not exactly sure how right now, and would rather spend my time today tackling the next issue. Please feel very welcome to make the change anytime if you want |
Not 100% sure if this architecture is sound or best way to do it... happy for any feedback.
A work in progress as I learn React and Redux.
Rel #127
Update:
I think it's pretty much to spec now, and seems functional in my own click-throughs.
@elaineo Note that I opted to use
openinstead ofmodalto allow the user to also outside-click/ESC to close the popup (modalrequires clicking a certain button). And thanks a lot for your feedback and guidance along the way.