-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix: cell errors #14168
fix: cell errors #14168
Conversation
1837b31
to
2e6202b
Compare
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.
Looks great. Left some inconsequential comments.
word-wrap: break-word; | ||
width: calc(100% - #{$box-tooltip--caret-size * 2}); | ||
|
||
pre { |
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 viewed the box tooltip as a generic container component that you could put anything inside. It seems like these additional .box-tooltip--contents
styles belong on the child component. What do you think? Want to keep the box tooltip / composable.
|
||
// If the width of the tooltip causes it to overflow left | ||
// position it to the right of the trigger element | ||
if (left < 0) { |
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.
nice. Think this makes the comment on line 36 out of date
@@ -44,7 +62,7 @@ const BoxTooltip: FunctionComponent<Props> = ({triggerRect, children}) => { | |||
|
|||
el.setAttribute( | |||
'style', | |||
`visibility: visible; top: ${top}px; left: ${left}px;` | |||
`visibility: visible; top: ${top}px; left: ${left}px; max-width: ${maxWidth}px;` |
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.
What if maxWidth
isn't passed? Is setting this to undefinedpx
ok?
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 that's probably fine, the browser will ignore it
testID?: string | ||
} | ||
|
||
const EmptyGraphError: FunctionComponent<Props> = ({message, testID}) => { |
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.
totally minor but name it the same as the filename?
</div> | ||
</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.
neat
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
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.
Looks good, only comment is we should have the copy button appear on hover of the text box so the text can fill the space.
Before
After