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

InputControl: Add optional Tooltip for inner input field #42726

Closed
wants to merge 6 commits into from

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Jul 27, 2022

Related:

What?

Adds the ability to provide a tooltip for the InputControl's inner input field.

Why?

This helps avoid the possibility of nested tooltips when the InputControl has an interactive prefix or suffix which might have its own tooltip.

How?

  • Adds new showTooltip & tooltipText props to the InputControl API
  • If showTooltip is true along with tooltipText or a string label, the inner InputField is wrapped in its own Tooltip
  • If rendering a Tooltip the InputField is wrapped in a div to avoid issues

Testing Instructions

  1. Open up Storybook
  2. Confirm the InputControl component displays the same on trunk as this PR (with or without showTooltip enabled)
  3. Enable showTooltip in the story controls and confirm the tooltip displays both on hover and via keyboard focus
  4. Provide a custom tooltipText value via story controls and confirm that text is used in the tooltip.
  5. Take a look at the other Storybook examples for InputControl and confirm Tooltips
  6. Check higher-level components that leverage InputControl still function/look correct e.g. UnitControl etc.

Screenshots or screencast

Screen.Recording.2022-08-01.at.5.21.34.pm.mp4
Original demo without custom tooltip text support
Screen.Recording.2022-07-27.at.5.19.01.pm.mp4

@aaronrobertshaw aaronrobertshaw added [Type] Enhancement A suggestion for improvement. [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components labels Jul 27, 2022
@aaronrobertshaw aaronrobertshaw self-assigned this Jul 27, 2022
Copy link
Contributor Author

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

This PR was drafted to explore options raised in #42661. It was also suggested that it might be beneficial for more than just the BorderBoxControl.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Code changes look nice and clean.

One aspect I also wanted to consider is that this approach goes a little bit against out general intentions of keeping low-level components modular and composable. I thought about exposing a renderInputWrapper prop that could be customised to include a Tooltip when needed, but if we think that the Tooltip functionality may be useful as a always-present feature of InputControl, then we should leave it as is.

Also, another thought: to avoid nested tooltips, we should find a way to only render this tooltip if there aren't any other parent Tooltips in the virtual DOM tree — which is being discussed in #42630

@ciampo ciampo requested review from mirka and afercia July 28, 2022 11:20
@aaronrobertshaw
Copy link
Contributor Author

but if we think that the Tooltip functionality may be useful as a always-present feature of InputControl, then we should leave it as is.

I think this could be handy to have around.

Also, another thought: to avoid nested tooltips, we should find a way to only render this tooltip if there aren't any other parent Tooltips in the virtual DOM tree — which is being discussed in #42630

Agreed. I'd like to tackle that within a follow-up once we have mechanism to achieve it e.g. a disable flag via context etc.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

While reviewing this PR, I also considered, as an alternative to exposing showTooltip and tooltipText, a more generic tooltipProps prop which we would just forward to the Tooltip component, but then I came to the conclusion that it's not ideal in this scenario, since we may not want to expose all Tooltip's props to the consumer of InputControl.

packages/components/src/input-control/types.ts Outdated Show resolved Hide resolved
packages/components/src/input-control/types.ts Outdated Show resolved Hide resolved
packages/components/src/input-control/types.ts Outdated Show resolved Hide resolved
@ciampo
Copy link
Contributor

ciampo commented Aug 10, 2022

I don't have any further feedback on this PR at the moment — I guess the next step would be to understand in #42661 if this is the way to go.

If that's the case, I'd argue that we may want to add some unit tests for tooltip functionality and mark this PR as ready for review — at that point, I'll give it a final round of review & testing.

@aaronrobertshaw
Copy link
Contributor Author

aaronrobertshaw commented Mar 21, 2024

Closing this old PR. If we come back to this or a similar approach it can be reopened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants