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

feat: react tooltips, tooltips for table #1546

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

samspi
Copy link
Contributor

@samspi samspi commented Aug 30, 2024

  • Added tooltip link component
  • Added tooltip button component
  • Implemented tooltip feature in Table

@green-design-system-bot
Copy link
Contributor

green-design-system-bot bot commented Aug 30, 2024

🦋 Changeset status

Based on the included changeset, the following releases will be published as a result of this PR:

  • @sebgroup/green-react: 3.14.1 → 3.15.0 (minor bump)

Note: If unpublished changesets where merged to main after the first commit of this PR, this
list will include those changes in addition to the ones specific to this PR.

@green-design-system-bot
Copy link
Contributor

green-design-system-bot bot commented Aug 30, 2024

👋 Thanks for creating a pull request!

🚀 Checkout the storybook we've created for it:

@splashdust splashdust self-assigned this Sep 2, 2024
@splashdust
Copy link
Contributor

@samspi Thank you for the PR!

I added a story for the tooltip and updated the docs for Table. Also, I removed the TooltipButton component (or rather moved it locally to table, where it is used internally), as I feel that it was a bit too specific to export as a public part of the library.

Copy link
Contributor

@vsjolander vsjolander left a comment

Choose a reason for hiding this comment

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

Hi and thank you for your contribution!

We (The Green Design System team) have found some issues that needs to be addressed:

Dynamic width

Long messages get very high:
Screenshot 2024-09-03 at 11 00 04

The width should be dynamic (follow the width of the text) but still follow the writing guidelines.

Accessibility issues

  • Tooltip only trigger on mouse hover - needs to support keyboard navigation
  • Tooltip is placed outside of screen, see storybook we are using floating UI to handle this.

@@ -124,7 +124,7 @@ export const SortableTable = () => {
<TableHeader>
<TableRow>
{columnData.map((column: SuperHeroColumnDataT, i: number) => (
<TableHeaderCell key={i} {...column} />
<TableHeaderCell key={i} {...column} tooltipText={'Tooltip'} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Its better to pass children to TableHeaderCell here:

<TableHeaderCell key={i} {...column}>
    <TooltipButton tooltip={tooltipText} className="sg-table-sort">
        {children}
    </TooltipButton>
</TableHeaderCell>

and remove the logic from the Table component. That way we keep components seperate and minimize dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! I'll get started on that!

Copy link
Contributor

Choose a reason for hiding this comment

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

How is this going @samspi? Can we assist you in any way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi there. An update. I've been unable to dedicate time to these fixes and I'm looking into handing it over to another member in the team. Thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants