Skip to content
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

Merged
merged 8 commits into from Jun 21, 2019
Merged

fix: cell errors #14168

merged 8 commits into from Jun 21, 2019

Conversation

121watts
Copy link
Contributor

@121watts 121watts commented Jun 20, 2019

Before

After

Kapture 2019-06-20 at 14 42 47

Copy link
Contributor

@chnn chnn left a 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 {
Copy link
Contributor

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) {
Copy link
Contributor

@chnn chnn Jun 20, 2019

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;`
Copy link
Contributor

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?

Copy link
Contributor

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}) => {
Copy link
Contributor

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>
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

neat

Copy link
Contributor

@Palakp41 Palakp41 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mavarius mavarius left a 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.

@121watts 121watts merged commit 5dfba47 into master Jun 21, 2019
@121watts 121watts deleted the fix/cell-errors branch June 21, 2019 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants