Setup dApp skeleton #7
Conversation
|
@liamzebedee Some summary in the PR description about what are we doing here would be helpful. |
|
Latest screenshots. The styling is very functional for now. I want to note that in |
|
Yeah so added some styling! Minor details missing ofc, dependent on how far we get with UX this week. But looking better
|
| @@ -0,0 +1,3227 @@ | |||
| # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
There was a problem hiding this comment.
Do we need this file? Aren't we using npm?
| @@ -0,0 +1,3 @@ | |||
| node_modules | |||
There was a problem hiding this comment.
Thoughts on moving this .gitignore to a project root directory?
| @@ -0,0 +1,38 @@ | |||
| order: | |||
There was a problem hiding this comment.
Maybe we should remove the plan from this PR?
| @@ -0,0 +1,24 @@ | |||
| { | |||
There was a problem hiding this comment.
We should commit package-lock.json.
| steps: | ||
| - checkout | ||
| - run: npm i | ||
| - run: npm run build |
There was a problem hiding this comment.
I'm not sure if this will work. Maybe we should drop the circleci configuration from this PR?
There was a problem hiding this comment.
Haha could you be less specific? Why won't it work?
There was a problem hiding this comment.
Both of you could use some details heh. Based on the commit that introduced this, I have no idea what you're trying to achieve. What's the purpose of adding this? That will probably help Kuba understand what is expected to work here.
It certainly doesn't test solidity, which is what your build currently implies 😉
There was a problem hiding this comment.
Haha could you be less specific? Why won't it work?
Yes, sorry. This will be executed in the root directory, which doesn't contain these commands definition. We should set working_directory for these steps.
We should also add workflows specification at the end of the file.
If we want to configure it all in this PR I can give you a hand with it.
There was a problem hiding this comment.
Closed but responding anyways - good catch guys, this was actually because I moved everything into app/ to play well with #8's file layout. ATT of that commit @Shadowfiend, it was correct and simply did a build.
| return { | ||
| type: SUBMIT_DEPOSIT_PROOF | ||
| } | ||
| } |
There was a problem hiding this comment.
How do you feel about using object literals w/ strings throughout the app?
I've always followed the pattern you have here, but I'm wondering if its really worth it. Action Creators are great for managing side-effects, but we now have sagas for that, so I'm questioning our assumptions a bit.
With action name constants, I still see value in those: easier to debug if you typo, but I am not convinced thats a mistake made often enough to justify the additional boilerplate.
What you have here is much cleaner/safer than what I'm suggesting, and is what I'm used to - I'm just wondering where your head is.
There was a problem hiding this comment.
I've never worked under a frontend senior, so I can't speak for the standard. But I wouldn't say the issue is typos - rather, it's refactors. In which case, using your type system is the right thing to do.
It's also something the official Redux docs recommend:
Encapsulating and centralizing commonly used pieces of code is a key concept in programming. While it is certainly possible to manually create action objects everywhere, and write each type value by hand, defining reusable constants makes maintaining code easier. If you put constants in a separate file, you can check your import statements against typos so you can't accidentally use the wrong string.
| import { AwaitDepositConfirmation } from '../components/AwaitDepositConfirmation' | ||
| import { waitConfirmation } from '../actions' | ||
|
|
||
| class AwaitDepositConfirmationContainer extends React.Component { |
There was a problem hiding this comment.
I like the container pattern, but questions whether we need it in an app this size. Each of these top-level components probably won't be re-used with different actions/state connected to it, so for readability maybe its worth colocating the connections.
There was a problem hiding this comment.
I'm interested if you could elaborate - what do you see as good frontend readability? :)
(I know we're not reviewing this PR anymore - but this is more about getting on the same page and learning from each other)
| if (!window[__NEXT_REDUX_STORE__]) { | ||
| window[__NEXT_REDUX_STORE__] = configureStore(initialState) | ||
| } | ||
| return window[__NEXT_REDUX_STORE__] |
There was a problem hiding this comment.
Is this redux store persisted in localstorage (or similar)? That'd be really really nice to have.
There was a problem hiding this comment.
for the demo, it doesn't seem relevant. Although totally on board with this and was looking at https://github.com/rt2zz/redux-persist ;)
| Router.push('/tbtc-minted') | ||
| } | ||
|
|
||
| export default function* () { |
There was a problem hiding this comment.
I really love having the takeEvery as simply part of the saga exports, rather than having a separate file of watchers. 👍
Iron Confederacy: Improve tbtc.js for usage by the tBTC reference dApp
Setup the dapp 'skeleton'
Closes: #5
Implements #2, but no test for the payment just yet (blocked by bitcoin testnet)