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

Add tooltips to overflow text on streams table #21253

Merged
merged 18 commits into from
Jan 19, 2023

Conversation

krishnaglick
Copy link
Contributor

@krishnaglick krishnaglick commented Jan 11, 2023

What

Resolves #20450
On the new streams table, fields that may have text that overflows now have a tooltip that shows the full text.

How

Adds a withTooltip property to the CatalogTreeTableCell component to enable the tooltip functionality.

https://www.loom.com/share/711bed4eb0434808a3f736cb861833ca

@octavia-squidington-iv octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 11, 2023
Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

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

So many tips!

A few notes:

  • My understanding is we only want the Tooltip if there is overflow, but maybe check in with Nico on that if you think I'm off there.
  • I did not see a Tooltip on the cursor and primary key cells.
  • I think all of our other Tooltips use a dark variant. I'm not sure if we want dark or light here? No strong feeling.

Left a few other, more specific thoughts, too!

@@ -80,9 +80,6 @@ export const CatalogTreeTableHeader: React.FC = () => {
<HeaderCell size="small">
<FormattedMessage id="sources.sync" />
</HeaderCell>
{/* <TextCell>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suppose this is fine to remove for now? We do want to have this field once we introduce column selection to the new table (#21058) we want this column. Unless the designs have changed and I'm not up to date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just removed it since it had been sitting around. I can re-add 👍

<Text size="md" className={styles.cellText}>
{stream.stream?.name}
</Text>
</CatalogTreeTableCell>
<CatalogTreeTableCell size="large">
<CatalogTreeTableCell size="large" withTooltip={disabled}>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of the main reasons we're introducing the Tooltip -- the sync mode "incremental | deduped + history" always has overflow. We want a Tooltip on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we want the value of the dropdown as a tooltip here?

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 was the idea as I understood it.

@krishnaglick
Copy link
Contributor Author

* I think all of our other Tooltips use a dark variant.  I'm not sure if we want dark or light here? No strong feeling.

I thought light looked better, but honestly they're both fine. I'll go w/ dark since that's the default.

Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

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

Nice! I did notice one case where if I resize the window, the text wraps, at least in this case (this is a source-defined primary key if the cell type matters). I'm not sure whether we want to solve that in this PR or if it's something for another PR.

Screenshot 2023-01-17 at 12 30 53 PM

@teallarson
Copy link
Contributor

teallarson commented Jan 19, 2023

Super close!
Still not seeing ellipses on overflowing source-defined primary keys. (EDIT: It did seem to do better when I resized the window. It cut off, though still no ellipses or tooltip, but for some reason at full width on my macbook I'm getting this mess)
Screenshot 2023-01-19 at 2 34 42 PM

Also, would be nice to parse the actual character here!
Screenshot 2023-01-19 at 2 25 34 PM

Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

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

Tested locally and both of the issues from earlier were resolved 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp team/platform-move
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All cells in streams/fields tables should have ellipses and tooltips for text overflow
3 participants