-
Notifications
You must be signed in to change notification settings - Fork 55
Add Tooltip
component
#360
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
Conversation
} | ||
} | ||
|
||
export default Tooltip; |
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.
Can you add prop-types too? I've been too lazy to do this on the other components, but in hindsight, I should have.
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.
Yeah, I didn't because no other components had it. Will add!
app/renderer/components/Tooltip.scss
Outdated
&__arrow { | ||
position: absolute; | ||
width: 16px; | ||
height: 24px; |
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.
Will make these variables.
<Manager> | ||
<Reference> | ||
{({ref}) => ( | ||
<div |
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.
Really bothers me that we have to wrap the children in a div
since it won't wrap elements with absolute position correctly. I'd really like to be able to pass a ref
to children directly. React.forwardRef()
only works for components.
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.
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.
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.
Ah, it's only available in Chrome 65, but Electron 2 is at 61. We'll be able to use this when Electron 3 is stable. #374
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 added the flag to blinkFeatures
, but it didn't work regardless, i.e. instead of getting the wrong position it got no position at all (which the screenshot shows).
@@ -5,43 +5,67 @@ import ReloadButton from 'components/ReloadButton'; | |||
import CopyButton from 'components/CopyButton'; |
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.
Had to rewrite this file to use flex
inside generated-seed-phrase-container
because we currently can't use absolutely positioned targets for our tooltips. flex
is nicer than using position: absolute
anyway :).
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.
Much better! No idea why I didn't use flexbox in the first place...
@sindresorhus, tried to, but didn't come up with any nice border or shadows. Does the animation look good though? Also, should the content animation be in this component or should we pass another component that animates the text as |
I think it needs something so it doesn't bleed into the background. Try with a slight shadow.
The animation looks good.
What you have now is good. We don't need that kind of manual control. |
@kevva I noticed that if I hover over the copy button, the click, then take my mouse somewhere else and then hover again, it still shows |
return ( | ||
<button | ||
{...props} | ||
type="button" | ||
className={`${className} CopyButton`} | ||
onClick={() => { | ||
onClick={e => { |
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.
e
=> event
app/renderer/components/Tooltip.scss
Outdated
@@ -0,0 +1,109 @@ | |||
.Tooltip { | |||
--color: var(--border-color); | |||
--arrow-height: 8px; |
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 would make this 7px
. It feels a tad bit too tall with 8px
.
<Manager> | ||
<Reference> | ||
{({ref}) => ( | ||
<div |
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.
@@ -5,43 +5,67 @@ import ReloadButton from 'components/ReloadButton'; | |||
import CopyButton from 'components/CopyButton'; |
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.
Much better! No idea why I didn't use flexbox in the first place...
class CreatePortfolioStep2 extends React.Component { | ||
state = { | ||
isCopied: 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.
Btw, check out https://github.com/hyperdexapp/hyperdex/blob/7d996b46533bc965409f53150b9b037731bc040c/app/renderer/containers/SuperContainer.js#L63-L74 It lets us add state to functional components. No more class mess when you only need a little bit of state.
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.
Sweet. Didn't notice that!
package.json
Outdated
"qrcode.react": "^0.8.0", | ||
"randoma": "^1.2.0", | ||
"react": "^16.3.2", | ||
"react-dom": "^16.3.2", | ||
"react-extras": "^0.6.0", | ||
"react-popper": "1.0.0-beta.6", |
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.
1.0.0
final is out
Added a slight shadow to it:
Should be fixed. Added an |
<div | ||
ref={arrowProps.ref} | ||
className="Tooltip__arrow" | ||
style={arrowProps.style} |
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.
Would've wanted to put these into styled-jsx
too, but that'd require converting them to a CSS string. Decided not to bother with it.
This looks and works perfectly now. Nice work Kevin :) |
top
For #175