Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wiring up new homepage to new dashboard #281

Merged
merged 7 commits into from
Sep 27, 2018
Merged

Wiring up new homepage to new dashboard #281

merged 7 commits into from
Sep 27, 2018

Conversation

benwerd
Copy link
Contributor

@benwerd benwerd commented Sep 26, 2018

No description provided.

@todo
Copy link

todo bot commented Sep 26, 2018

change account to base level

account: UnlockPropTypes.account,
network: UnlockPropTypes.network,
transactions: UnlockPropTypes.transactions,
locks: UnlockPropTypes.locks,
}


This comment was generated by todo based on a TODO comment in b96c624 in #281. cc @unlock-protocol.

Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

A lot of the logic here should be handled by react-router.

render() {
if (!this.props.account) {
return null //loading
}
Copy link
Member

Choose a reason for hiding this comment

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

We should probably eventually handle that at a higher level.. But that'll do for now!

}

const mapDispatchToProps = dispatch => ({
setTransaction: (transaction) => dispatch(setTransaction(transaction)),
Copy link
Member

Choose a reason for hiding this comment

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

Are we actually calling this anywhere in the component?


dashboardClick() {
this.props.history.push('/dashboard')
}
Copy link
Member

Choose a reason for hiding this comment

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

Hum, I would expect react router to handle that for you?

@@ -13,7 +23,7 @@ export default class Home extends PureComponent {
Unlock is a protocol which enables creators to monetize their content with a few lines of code in a fully decentralized way.
</Headline>

<Action>
<Action onClick={this.dashboardClick}>
<HomepageButton>Go to Your Dashboard</HomepageButton>
Copy link
Member

Choose a reason for hiding this comment

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

Same here, you should probably use Link from react-router-dom.

Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

I checked things and I really think you should use react router's stuff for this. LMK if you need help with it though!

@benwerd
Copy link
Contributor Author

benwerd commented Sep 27, 2018

Yep, I'm working on the changes!

@benwerd
Copy link
Contributor Author

benwerd commented Sep 27, 2018

Using router. PTAL

<HomepageButton>Go to Your Dashboard</HomepageButton>
<ButtonLabel>Requires a browser with an Ethereum wallet</ButtonLabel>
</Action>
<Link to={'/dashboard'}>
Copy link
Member

Choose a reason for hiding this comment

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

You'll know what I say about nesting components!... but I guess you tried without?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did, yeah. I have some feature requests for the next version of styled-components.

Copy link
Member

Choose a reason for hiding this comment

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

lol. happy to hear them!

Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

2 small nits!

@@ -51,6 +55,10 @@ export default class Home extends PureComponent {
}
}

Home.propTypes = {
Copy link
Member

Choose a reason for hiding this comment

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

You can probably get rid of this ;)

@benwerd benwerd merged commit 78e1589 into master Sep 27, 2018
@benwerd benwerd deleted the ben-newdesign branch September 27, 2018 19:50
@todo todo bot mentioned this pull request Sep 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants