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

Feature/add tooltip component #176

Merged
merged 2 commits into from
Sep 15, 2023
Merged

Feature/add tooltip component #176

merged 2 commits into from
Sep 15, 2023

Conversation

celineung
Copy link
Collaborator

Add tooltip component

See DSFR doc

@celineung celineung force-pushed the feature/tooltip-component branch 3 times, most recently from b06bde1 to 6b51edf Compare August 29, 2023 16:02
@celineung celineung marked this pull request as ready for review August 29, 2023 16:03
@celineung celineung force-pushed the feature/tooltip-component branch 2 times, most recently from 4c3a407 to 3d1672c Compare August 30, 2023 09:48
src/Tooltip.tsx Outdated Show resolved Hide resolved
@celineung celineung force-pushed the feature/tooltip-component branch from ee676fb to 4e8ec02 Compare August 30, 2023 13:48
Copy link
Collaborator

@garronej garronej left a comment

Choose a reason for hiding this comment

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

Hey @celineung,

Thank you for your work! 😊 here is my review.

src/Tooltip.tsx Outdated Show resolved Hide resolved
src/Tooltip.tsx Outdated
"aria-describedby": string;
className?: string;
};
TooltipOwnerButtonComponent?: JSX.Element;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
TooltipOwnerButtonComponent?: JSX.Element;
button?: JSX.Element;

This is not a component; it's a node. Therefore, it shouldn't be capitalized.

Additionally, I think the API could benefit from a redesign. Whether the button is undefined or not should be inferred from the parameters. If a button exists, then tooltipOwnerProps has no reason to be prsent on the return type. Also, it's somewhat confusing from the user's perspective to have to both mount a node and a component. Ideally, they should just have to use a single TooltipButton with a label and a tooltip prop or something similar.

When designing an API, it's crucial to consider the user's perspective. What makes the most sense for them. What design would constiture a better (DX). The implementation details are secondary; the primary focus should be on fulfilling the API design.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @garronej, thank you for your review :)
I updated the PR and made a new suggestion for the Tooltip Component.

src/Tooltip.tsx Outdated Show resolved Hide resolved
src/Tooltip.tsx Outdated Show resolved Hide resolved
@celineung celineung force-pushed the feature/tooltip-component branch from 4e8ec02 to 461a3a4 Compare September 5, 2023 15:46
@JeromeBu
Copy link
Collaborator

Hi @garronej , I think @celineung has taken your suggestions into account and is waiting for your review

@garronej
Copy link
Collaborator

Oh yes sorry!
I'll look at this today

@garronej
Copy link
Collaborator

garronej commented Sep 15, 2023

Hi @celineung,

Thank you for your contribution. The idea of applying the id to child elements is more effective than my initial suggestion. I also appreciate the inclusion of type-level tests.
I've merged and then rebased the changes, as there are several points we need to address:

  • Instead of wrapping everything in a ``, we should eliminate the wrapper altogether. The `className` and `id` should be applied directly to `fr-tooltip`, and the root should be a fragment (`<>...</>`). Beside wrapping in a span affects the color of the link.
  • There's no demonstration story for using this with a link.
  • The code contains much duplication and needs to be refactored.
  • The `displayChildren` function takes arguments that are already in scope, making it redundant. It's also immediately called with the same arguments, so it should be a node, not a function.
  • Hardcoding the string "Information contextuelle" in French is not acceptable; we should use our i18n module. Additionally, this string should be configurable.
  • Always use `fr.cx()` to ensure type safety and protect against silent regressions.
  • There are variables that are assigned but never used.
  • For consistency, let's use `ReactNode` instead of `ReactElement`.
  • Import all types as types.
  • The `description` field could be a `ReactNode`; there's no reason to limit it.
  • To align with MUI conventions, the `description` field should be renamed to `title`.
  • The `style` tag is missing, which is a standard element in all our components.

I've opened a new PR.

Thanks for your efforts on this. If you have the time and interest to make these adjustments, that would be great. However, I understand that open-source contributions can be time-consuming, so no pressure to follow up if you can't. Your willingness to contribute is already appreciated.

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.

4 participants