Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Add adress info Component#26

Merged
nicosampler merged 25 commits intodevelopmentfrom
adress-info
Jul 30, 2020
Merged

Add adress info Component#26
nicosampler merged 25 commits intodevelopmentfrom
adress-info

Conversation

@alongoni
Copy link
Contributor

@alongoni alongoni commented Jun 26, 2020

Closes #23.

@alongoni alongoni self-assigned this Jun 26, 2020
@ghost
Copy link

ghost commented Jun 26, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@ghost
Copy link

ghost commented Jun 26, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@ghost
Copy link

ghost commented Jun 26, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@nicosampler nicosampler self-assigned this Jul 17, 2020
@github-actions
Copy link

github-actions bot commented Jul 27, 2020

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@ghost
Copy link

ghost commented Jul 27, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@ghost
Copy link

ghost commented Jul 27, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@ghost
Copy link

ghost commented Jul 28, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@nicosampler nicosampler marked this pull request as ready for review July 28, 2020 13:25
const EllipsisMenu = ({ menuItems }: Props): React.ReactElement => {
const [anchorEl, setAnchorEl] = React.useState(null);

const handleClick = (event: any) => setAnchorEl(event.currentTarget);
Copy link
Contributor

Choose a reason for hiding this comment

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

I could not add the type, perhaps someone could help me with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

something around...

  const handleClick: React.MouseEventHandler<'onclick'> = (event) => setAnchor(event.currentTarget);

seems to work.

Probably too specific, but in this case, I think it's enough.

Copy link
Contributor

@nicosampler nicosampler Jul 28, 2020

Choose a reason for hiding this comment

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

it does not work for me

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot from 2020-07-29 14 59 24

Material UI docs have both JS/TS examples with types 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

I know and I already test it. But didn't work.

image

Could you try yourself? Perhaps it's a problem with my computer.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you read through error message it says that it cannot assign to type of null (which is the inferred type of anchorEl). I already pushed a fix

@nicosampler
Copy link
Contributor

nicosampler commented Jul 28, 2020

once CopyToClipboardBtn component is merged, I have to replace the fixed icon "copy" by the component.

@ghost
Copy link

ghost commented Jul 28, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@ghost
Copy link

ghost commented Jul 28, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@ghost
Copy link

ghost commented Jul 29, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

<AddressInfo address={address} shortenAddress={4} />
);

export const WithOwner = (): React.ReactElement => (
Copy link
Contributor

Choose a reason for hiding this comment

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

WIth a name, not necessarily an owner

address={address}
name="Owner 1"
showIdenticon
showCopy
Copy link
Contributor

Choose a reason for hiding this comment

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

showCopyWhat?

name="Owner 1"
showIdenticon
showCopy
showEtherscan
Copy link
Contributor

Choose a reason for hiding this comment

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

showEtherscanWhat?

showEtherscan?: boolean;
};

const getShortAddress = (text: string, shortenAddress: number) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this wrapper over wrapper is unnecessary, you could use textShortener just fine

{ label: 'Item 2', onClick: console.log },
];

export const Loader = (): React.ReactElement => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this called Loader?

Comment on lines +14 to +16
:focus {
outline: none;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@ghost
Copy link

ghost commented Jul 29, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

1 similar comment
@ghost
Copy link

ghost commented Jul 29, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@ghost
Copy link

ghost commented Jul 29, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@fernandomg
Copy link
Contributor

shouldn't this be closing #23 instead of #40?

@alongoni, @nicosampler

@ghost
Copy link

ghost commented Jul 29, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@ghost
Copy link

ghost commented Jul 29, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@ghost
Copy link

ghost commented Jul 29, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@ghost
Copy link

ghost commented Jul 29, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@ghost
Copy link

ghost commented Jul 29, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

Copy link
Contributor

@mmv08 mmv08 left a comment

Choose a reason for hiding this comment

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

Please fix those before merging

@@ -29,12 +30,13 @@ const StyledButtonLink = styled.button<Props>`
const ButtonLik = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

keepMounted
onClose={closeMenuHandler}
open={Boolean(anchorEl)}>
{menuItems.map((item, i) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

i is unused

@ghost
Copy link

ghost commented Jul 30, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@ghost
Copy link

ghost commented Jul 30, 2020

Travis automatic deployment:
https://pr26--safereactcomponents.review.gnosisdev.com

@nicosampler nicosampler merged commit 2b4635b into development Jul 30, 2020
@nicosampler nicosampler deleted the adress-info branch July 30, 2020 13:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create component to display address info

4 participants