Landing #7

tynes · 2018-09-11T17:33:09Z

Basic Overview container component, without styling
This PR is to determine if the file structure is set up nicely
Renders side bar
Some basic clicky functionality with some buttons

Meant to be this view:

Where there is 1 container and 3 components that go with it, the TxList component will be the most reusable

bucko13 · 2018-09-11T20:25:57Z

What do you think of renaming Multisig Init? It doesn't feel super descriptive to me. Without the content of the table it took me a minute to think what it was going to be. @DarrenRM @pinheadmz any thoughts? Some ideas (a word in brackets is for an idea with or without that word):

Pending Multisig Wallets
[Pending] Multisig Invitations

Maybe also rename the init column to "Members" or "Joined" or something like that?

pinheadmz · 2018-09-11T20:32:57Z

I like Pending Multisig or Pending Multisig Wallets -- does it need to be any more novice level than that? (multi-signature? multi-party? shared wallet?)

@tynes What happens when all participants join the wallet? Does it pop up to the Wallets List? Or is there still a separate Multisig section?

bucko13 · 2018-09-11T20:34:16Z

lib/Containers/MainContainer.js


 export default class MainContainer extends PureComponent {
  constructor(props) {
    super(props);
  }

  render() {
+    const { match } = this.props;
+
+    // TODO: figure out how to get subitems on sidebar to work


Hopefully the docs help with this: https://bpanel.org/docs/ui-utilities.html#createnestedviews
If there's anything that's not clear, overlooked, or could be clarified, let me know! (or submit as PR to bpanel-docs 😁). There's also a sample implementation in the @bpanel/bui plugin.

It looks like you're basically there though!

tynes · 2018-09-11T20:56:47Z

@pinheadmz To my understanding, the Multisig Init table was for pending multisig wallets that have yet to be fully joined, so a redeem script hasn't been generated yet for them

If that is what the table is supposed to be, then i'd imagine that when a wallet is fully joined, it would no longer exist in that list. The one problem that we have right now is that we do not have websockets set up for wallet join

tynes · 2018-09-11T20:57:50Z

I like Pending Multisig Wallets

bucko13 · 2018-09-11T21:00:35Z

That's a great point about making it even more accessible, in which case Multi-party feels more like something my parents (for example) would be able to understand and still conveys the original meaning (and would even cover the situation with 1 individual, multiple devices). Not sure if it's a pre-emptive over-optimization, since we'll be a long way off from getting novices in at that level, but then again, maybe you can never start with ease-of-use too early.

DarrenRM · 2018-09-11T21:09:23Z

Pending Multi-party Wallets or Pending Multisig Wallets is definitely way better. As for which, I think I lean slightly towards Multi-party since it's really simple to understand but you probably get it if you understand what multisig is.

tynes · 2018-09-12T00:12:59Z

Linting is failing, will return to this once we have our meeting on the design

bucko13 · 2018-09-12T18:53:50Z

lib/constants/index.js

+const REDUCE_INTERFACE_NAMESPACE = '@bpanel/bwallet/interface';
+const REDUCE_NODE_NAMESPACE = '@bpanel/bwallet/node';
+const REDUCE_CHAIN_NAMESPACE = '@bpanel/bwallet/chain';
+const REDUCE_WALLETS_NAMESPACE = '@bpanel/bwallet/wallets';


Should the plugin namespace just be used as a constant with template literals here?

Also wondering about the decision to have each of these top level rather than basically as child stores under the namespace? I could see this ending up making the plugins store much more cluttered

How do I join multiple reducers into 1 namespace?

nested reducers and/or combineReducers:
reduxjs/redux#738
http://www.ematipico.com/javascript/redux/nested-reducers/

It's a little obfuscated but this is how we actually manage the plugin reducers in bpanel to enable for the namespaced reducers nested in the plugins reducer: https://github.com/bpanel-org/bpanel/blob/development/webapp/plugins/plugins.js#L263

Maybe this would be helpful to add to the reducers documentation... 🤔

bucko13 · 2018-09-13T17:01:25Z

lib/components/PendingMultisig.js

+import { connect } from 'react-redux';
+import { Header, Table } from '@bpanel/bpanel-ui';
+
+class PendingMultisig extends PureComponent {


I'm not sure which way is best myself, I could see the argument for either, but do you think it makes to switch this component to be called PendingMultiparty as well to match what the UI displays?

We will probably have to change it again if we always change every name based on the UI. I think its close enough

What do you mean? Ideally the UI doesn't change if at all once we've stabilized. Just thinking about the situation where someone wants to make a change to the PendingMultiparty component and can't find it immediately. It's not hard to deduce of course, but it's the convention we use with virtually every other component right?

Updating to PendingMultiparty, I meant that until after we have our design meetings, the names could be unstable

tynes · 2018-09-17T20:26:35Z

@bucko13 - ready for a lookthrough

tynes · 2018-09-17T20:28:55Z

lib/components/PendingMultiparty.js

+    this.COL_HEADERS = ['Name', 'Date', 'M of N', 'Init'];
+  }
+
+  componentDidMount() {


tynes · 2018-09-17T20:29:10Z

lib/components/RecentTxList.js

+    this.COL_HEADERS = ['Date'];
+  }
+
+  componentDidMount() {


bucko13 · 2018-09-17T21:23:20Z

lib/components/index.js

@@ -0,0 +1,19 @@
+import WalletsList from './WalletsList';


Small styling nit, more of a personal style so feel free to ignore, but you could simplify this file with: export { default as WalletsList } from './WalletsList'; This only works for default exports though

bucko13 · 2018-09-17T21:25:53Z

lib/constants/index.js

@@ -10,27 +10,47 @@
 * in the redux store in case
 * of collisions with other plugins
 */


If we're going to keep all constants in a single file, does it need to be in a directory? Could it just be a single constants.js file?

There are multiple files in the constants directory. Do you think it should be modularized more? There are some defined in index, but also others defined in the other files

Yeah, I think so. I feel like indexes should either be fully an "entrypoint" or fully code logic. Feels easier to reason about.

bucko13 · 2018-09-17T21:30:02Z

lib/containers/SendReceive.js

+    return (
+      <div>
+        <Header type="h5">Select Wallet</Header>
+        <Dropdown options={[]} placeholder={'Select Wallet'} />


when a prop is just a string, I believe you can just pass it without the curly braces: placeholder="Select Wallet" same as we do for classNames.

bucko13 · 2018-09-17T21:36:38Z

lib/mappings/overview.js

+} from './selectors';
+
+// TODO: append proper network symbol to balance based on chain
+function buildWalletsList(wallets, multisigWallets) {


So this is basically the ideal use case of reselect and selectors since you're going through the stores and re-shaping the data based on a specific use case. I think what will likely happen in this implementation is that every time walletsList is retrieved through the mapStateToProps below is that this function will be called, which will go through the for loops. If you use reselect (which is available as a peerDependency too for plugins), then the walletsList will be memoized and returned, only recalculated if the state gets updated.

bucko13 · 2018-09-17T22:52:04Z

lib/mappings/mainContainer.js

+function mapDispatchToProps(dispatch) {
+  return {
+    selectAccount: async (walletId, accountId) =>
+      dispatch(selectAccount(walletId, accountId)),


You shouldn't need the async here since there's not an await inside this function (there is in selectAccount itself, but that already has the async in the containing function declaration). when selectAccount is called later on, the containing function will have an async and then you would have something like const acct = await selectAccount.

tynes · 2018-09-19T21:26:35Z

lib/mappings/mainContainer.js

+  };
+}
+
+// TODO: be certain we need selectAccount and selectWallet


looks like we don't

should this comment be cleaned up then?

tynes · 2018-09-19T21:27:29Z

lib/containers/SendReceive.js

+
+  render() {
+    const { wallets, accounts, selectedWallet, selectedAccount } = this.props;
+    const tabs = [


Move tabs into own function

tynes · 2018-09-19T21:29:12Z

This should fix the infinite loop bug and is ready to be looked at again @bucko13

tynes requested a review from bucko13 September 11, 2018 17:33

bucko13 reviewed Sep 11, 2018

View reviewed changes

tynes force-pushed the landing branch from 78c4d4c to eb06205 Compare September 11, 2018 22:46

bucko13 reviewed Sep 12, 2018

View reviewed changes

bucko13 reviewed Sep 13, 2018

View reviewed changes

tynes commented Sep 17, 2018

View reviewed changes

lib/components/RecentTxList.js Outdated

this.COL_HEADERS = ['Date'];

}

componentDidMount() {

Copy link

Collaborator Author

tynes Sep 17, 2018

Choose a reason for hiding this comment

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

delete

bucko13 reviewed Sep 17, 2018

View reviewed changes

mark tyneway added 12 commits September 19, 2018 14:06

deps: add bpanel ui

2c206e8

progress: flesh out landing page

4ba845c

components: build out scaffold

d13c032

components: basid overview page

849bc78

components: rename, more skeleton

7712d9d

refactor: lowercase directory

f719adc

redux: hook up

302df09

components: more skeleton

24b0f9a

lint: run lint-fix

38613c6

bugfix: define payload

76f55c9

lint

f292a35

refactor: modularize, clean

fc46075

mark tyneway added 24 commits September 19, 2018 14:21

lint: lint fix

2c4176d

refactor: modularize

bdec7d4

containers: init state set

65cbf35

bugfix: better handle multisig

f723782

lint

3f46315

refactor: delete dead code

5acb64a

refactor: clean up exports

ec773fe

bugfix: fetch wallets, set accounts

d2b5706

refactor: modularize

0b37252

deps: add reselect

35e5f0b

deps: add lodash

f9f9201

bugfix: remove async keyword

fcce895

bugfix: remove async

771e372

refactor: remove dead code, return copied state

a49908e

mappings: attempt cache

85499ef

progress: create multisig

6447f52

container: logic for wallet selection

abfd271

components: send funds basic styles

af6b08b

progress: side nav lists wallets

38abbd2

debug: remove console.log

7612c04

container: render using side bar

65959b8

lint: fix

8cf1849

bugfix: infinite async loop

ec8dfb3

lint: lint-fix

ad5f5ca

tynes force-pushed the landing branch from 1e070df to ad5f5ca Compare September 19, 2018 21:24

tynes commented Sep 19, 2018

View reviewed changes

bucko13 approved these changes Sep 20, 2018

View reviewed changes

tynes merged commit 44f999e into master Sep 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Landing #7

Landing #7

tynes commented Sep 11, 2018 •

edited

bucko13 commented Sep 11, 2018

pinheadmz commented Sep 11, 2018 •

edited

bucko13 Sep 11, 2018

bucko13 Sep 11, 2018

tynes commented Sep 11, 2018

tynes commented Sep 11, 2018

bucko13 commented Sep 11, 2018

DarrenRM commented Sep 11, 2018

tynes commented Sep 12, 2018

bucko13 Sep 12, 2018

tynes Sep 13, 2018

bucko13 Sep 13, 2018

bucko13 Sep 13, 2018

bucko13 Sep 13, 2018

tynes Sep 13, 2018

bucko13 Sep 13, 2018

tynes Sep 14, 2018

tynes commented Sep 17, 2018

tynes Sep 17, 2018

tynes Sep 17, 2018

bucko13 Sep 17, 2018

bucko13 Sep 17, 2018

tynes Sep 17, 2018

bucko13 Sep 17, 2018

tynes Sep 18, 2018

bucko13 Sep 17, 2018

bucko13 Sep 17, 2018

bucko13 Sep 17, 2018

tynes Sep 19, 2018

bucko13 Sep 20, 2018

tynes Sep 19, 2018

tynes commented Sep 19, 2018

Landing #7

Landing #7

Conversation

tynes commented Sep 11, 2018 • edited

bucko13 commented Sep 11, 2018

pinheadmz commented Sep 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tynes commented Sep 11, 2018

tynes commented Sep 11, 2018

bucko13 commented Sep 11, 2018

DarrenRM commented Sep 11, 2018

tynes commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tynes commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tynes commented Sep 19, 2018

tynes commented Sep 11, 2018 •

edited

pinheadmz commented Sep 11, 2018 •

edited