Add tooltip to contacts #8974
Add tooltip to contacts #8974
Conversation
There was a problem hiding this comment.
Thanks @muzakparov! Looks pretty good overall - I had a few comments/questions.
Not sure if it is necessary, but I think that copy logic could be abstracted to hooks since it seems to repeat in different places.
Definitely not necessary, but I agree that would be great. We'd just need to convert these to functional components first, so we could use hooks.
| onClick={() => copyToClipboard(text)} | ||
| onClick={() => { | ||
| this.setState({ copied: true }) | ||
| setTimeout(() => this.setState({ copied: false }), 3000) |
There was a problem hiding this comment.
This will throw an error in the console if this component is unmounted when this timer resolves. It's a harmless error in this case, but still nice to avoid. It could be avoided by saving the timeout ID, and clearing the timeout on unmount.
There was a problem hiding this comment.
The same applies to the similar change made to the ViewContact component as well
ui/app/pages/settings/contact-list-tab/view-contact/view-contact.component.js
Outdated
Show resolved
Hide resolved
| className="address-book__view-contact__group__static-address--copy-icon" | ||
| onClick={() => copyToClipboard(checkSummedAddress)} | ||
| <Tooltip | ||
| wrapperClassName="address-book__tooltip-wrapper" |
There was a problem hiding this comment.
It looks like this class doesn't exist? 🤔 If so, better to not reference it.
| wrapperClassName="address-book__tooltip-wrapper" |
|
thank you @Gudahtt Locally, I tried merging and then linting, there were no changes, so I haven't pushed the last commit. |
|
Hey @muzakparov, we only just merged the PR that included uncommenting that linting rule. After you update this branch, it should work properly if you try linting again. Sorry for the confusion! |
|
Cool stuff! I merged with your changes and seems to be fine now. Also, github seems to be having some synchronization issues at the moment, so I am unable to see my changes on the Pull Request. I will try again tomorrow |
|
Linting seems a bit different on CI/CD, I had to add a comma to make the linting test pass. |
|
Oh, that might be because we recently updated |
|
I merged with develop branch, didn't notice that I had many commits sorry |
|
hi @Gudahtt could you please help me with the error:) not urgent |
|
Sure, no problem! I'll take a look |
|
I'm not sure exactly what went wrong, but I backtracked and merged Sorry for the delay in reviewing this - I'm hoping to get to it today or tomorrow. |
ui/app/components/ui/export-text-container/export-text-container.component.js
Outdated
Show resolved
Hide resolved
muzakparov
force-pushed
the
tooltip-address
branch
from
2c68f3a to
bf4ff1c
Compare
Related to #8790
Contact page:
Seed phrase page:
Not sure if it is necessary, but I think that copy logic could be abstracted to hooks since it seems to repeat in different places.