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

Initial commit for creator lock component and stories #244

Merged
merged 3 commits into from
Sep 13, 2018

Conversation

benwerd
Copy link
Contributor

@benwerd benwerd commented Sep 13, 2018

This PR contains the saved and confirming states for the lock from the creator side.

There are a few known issues that will need to be added in a following PR:

  • The edit and code icons are broken
  • All icons are missing their grey background
  • The icon bar on saved locks does not show/hide

I've also added TODOs for the exchange rate values of the lock, and have kept the styles for those values commented out for now.

@todo
Copy link

todo bot commented Sep 13, 2018

add USD values to lock

// TODO add USD values to lock
// TODO add all-time balance to lock
return (
<CreatorLockRow>
<CreatorLockSaved>
<CreatorLockIcon><Icon lock={lock} address={lock.address} size={'3'} /></CreatorLockIcon>


This comment was generated by todo based on a TODO comment in 56a7d2c in #244. cc @unlock-protocol.

@benwerd
Copy link
Contributor Author

benwerd commented Sep 13, 2018

screen shot 2018-09-13 at 1 38 52 pm

screen shot 2018-09-13 at 1 38 00 pm

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 few changes (the big one being that we cannot change the icon components... because they are generated from the SVG)

// TODO add USD values to lock
// TODO add all-time balance to lock
return (
<CreatorLockRow>
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth splitting that in multiple files eventually? All importing the same elements from below?

Copy link
Member

Choose a reason for hiding this comment

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

Also, not sure it is worth prefixing Creator on all of these components?

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 wanted to keep the namespace clear for user-facing components, as these are all creator-facing UI elements. But I could change some of the names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could totally split this up into multiple files though.

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'll do exactly that in a follow-up PR.

Copy link
Member

Choose a reason for hiding this comment

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

About the naming, I think one of the great thing about the styled components approach is that everything is scoped basically. Thesr components should but be reused.

export function CreatorLock({ lock, status = 'deployed' }) {
// Some sanitization of strings to display
let name = lock.name || 'New Lock'
let displayAddress = lock.address ? lock.address.substr(0, 16) : ''
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn;t it be better to use CSS for this?

const inWei = Web3Utils.toWei(amount || '0', unit)
const inEth = Web3Utils.fromWei(inWei, 'ether')
return (<span>Ξ {inEth}</span>)
export function Balance({ amount, unit = 'wei', symbol=true }) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind adding a test for this? (I think the Balance component has tests?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -1,7 +1,7 @@
import React from 'react'

const Eth = props => (
<svg viewBox="0 0 7 11" width="1em" height="1em" {...props}>
Copy link
Member

Choose a reason for hiding this comment

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

nope... these are automatically generated so we cannot change the values in there (I need to find a way to indicate that).
If you need the icons to be larger just change the font-size of the element in which the icons are displayed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah got it. OK!

@benwerd
Copy link
Contributor Author

benwerd commented Sep 13, 2018

Updated. New screenshots:

screen shot 2018-09-13 at 1 54 41 pm

screen shot 2018-09-13 at 1 54 36 pm

@benwerd
Copy link
Contributor Author

benwerd commented Sep 13, 2018

Okay. Tests and reformatting completed. I'll follow up with splitting up into smaller components. PTAL.

@benwerd benwerd merged commit 809b07a into master Sep 13, 2018
@todo todo bot mentioned this pull request Sep 13, 2018
@benwerd benwerd deleted the ben-creator-lock branch September 13, 2018 21:50
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