803 advanced organization settings #23
Conversation
chadoh
force-pushed
the
803-advanced-organization-settings
branch
from
dc5a877 to
9bbb45a
Compare
chadoh
left a comment
chadoh
left a comment
There was a problem hiding this comment.
This is coming along nicely! I don't see any big issues, but given that there's a lot of code I saw a bunch of small things we might want to clean up.
src/App.js
Outdated
| /> | ||
| </div> | ||
|
|
||
| <HelpScoutBeacon locator={locator} apps={apps} /> |
There was a problem hiding this comment.
Why did this go away? I don't see it added anywhere else in this PR.
| tokenDescription: 'This is a very unethical token.', | ||
| tokenAddress: '0x5Fef2867d4BbF1E7423Bb7FB9031402D4F99d2b0', | ||
| }, | ||
| ] |
There was a problem hiding this comment.
I love this placeholder data 😄
Do we want to ship it like this for now, or is there other work that needs to happen here?
| const OpenAppButton = props => <Link css="font-weight: 600" {...props} /> | ||
|
|
||
| export default Organization | ||
| export default props => { |
There was a problem hiding this comment.
In open-enterprise, our linter yells at us if we export anonymous functions like this. The thinking is that anonymous functions aren't as helpful when debugging. Even if the linter in this project doesn't do that, I think it seems nice to follow that rule. Two options for following it:
// option 1
export default function OrganizationWithLayout (props) {
...
}
// option 2
const OrganizationWithLayout = props => {
...
}
export default OrganizationWithLayout|
|
||
| const Section = ({ ...props }) => { | ||
| return <Box padding={3 * GU} {...props} /> | ||
| } |
There was a problem hiding this comment.
I vote we either:
- stop using this; it's only used in two places right now and it's pretty simple
- move it to its own file, and use it in all these new components
Option 2 feels a little bit silly, because it's such a small component. On the other hand, it would give us a single place where we could adjust that 3 * GU parameter, if we want to.
I don't know which of these options I prefer.
| <p | ||
| css={` | ||
| ${textStyle('body2')} | ||
| `} |
There was a problem hiding this comment.
I see this didn't change, but since git's diffing tools aren't helpful in showing that, I wonder if now is a good time to simplify this to something like:
<p css={textStyle('body2')}>| <Label text="Placeholder image" /> | ||
|
|
||
| <div css="margin-bottom: 20px"> | ||
| <Dropzone onDrop={acceptedFiles => console.log(acceptedFiles)}> |
There was a problem hiding this comment.
I think it would increase the visibility of this placeholder function if we move it up next to the saveColors definition.
| <label | ||
| css={` | ||
| ${textStyle('label2')}; | ||
| ${unselectable()}; |
| `} | ||
| > | ||
| {text} | ||
| {children && children} |
There was a problem hiding this comment.
If the first value is truthy, the && tells JavaScript to return the second value:
If the first value is falsey, the && tells JS to return the first value, without even checking the second:
All this to say: I don't think children && children behaves any differently from just a single children.
| <div | ||
| css={` | ||
| width: ${oneColumn ? '100' : '50'}%; | ||
| padding-left: ${oneColumn ? '0' : '20px'}; |
There was a problem hiding this comment.
There are definitely ways to accomplish all of this with CSS; no need to rely on JS to tell us if it's one column. I'm not sure if it's worth the effort for now, but it might be a good way to learn some CSS together, @rkzel.
| padding-left: ${oneColumn ? '0' : '20px'}; | ||
| `} | ||
| > | ||
| <Dropzone onDrop={acceptedFiles => console.log(acceptedFiles)}> |
There was a problem hiding this comment.
Let's move this placeholder function to the top of the component for visibility.
chadoh
force-pushed
the
803-advanced-organization-settings
branch
from
9bbb45a to
c7f4c85
Compare
This PR is for extending FE of Settings (recently renamed to Organization) - Tabs, logos, colors, etc. It does not contain integration with IPFS - this will need to be done separately.