-
Notifications
You must be signed in to change notification settings - Fork 23
refactor: Use function components, remove React class #355
Conversation
7737363
to
7ef88ff
Compare
}; | ||
export function CopyButton (props: CopyButtonProps) { | ||
const { value } = props; | ||
const [copied, setCopied] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like you're specifying the state types in other components, so even though it's obvious here what the types are I think it'd be worth doing so here as well for consistency. useState<boolean>(false)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh I'd prefer not to put explicitly. TS type inferring works pretty well, and someone seeing useState(false)
knows immediately it's a boolean.
The reason why I put in some cases is that TS cannot infer the type, and needs a hint.
@@ -6,21 +6,19 @@ import React from 'react'; | |||
import { Form } from 'semantic-ui-react'; | |||
import SUITextArea from 'semantic-ui-react/dist/commonjs/addons/TextArea'; | |||
|
|||
type Props = { | |||
export type TextAreaProps = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't personally see a need to export this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was maybe anticipating a scenario that won't happen:
import { BalanceDisplay, BalanceDisplayProps} from '@substrate/ui-components';
const MyBalance = styled<BalanceDisplayProps>(BalanceDisplay)`
// some css stuff
`;
or interface MyBalance extends BalanceDisplayProps;
anyways, removed
packages/ui-components/src/stateful/AddressSummary/AddressSummary.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
several small comments, overall looks awesome though. I'm excited to be using more fp-ts
!
Let's ignore codeclimate issues for this one, I'll focus on them in the next PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I didn't do the ones in Onboarding. We could maybe do them with #67