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

Toggletip: Add support to programmatically close it #75846

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

adela-almasan
Copy link
Contributor

@adela-almasan adela-almasan commented Oct 2, 2023

As part of the WIP DashGPT project, we needed to be able to close the Toggletip after clicking a button. I've added some changes to reflect that functionality, I'd appreciate feedback. Being optional, I don't believe the change should be disruptive of the current functionality.

Extracted from #75204

toggletip.mov

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@adela-almasan adela-almasan added this to the 10.2.x milestone Oct 2, 2023
@adela-almasan adela-almasan self-assigned this Oct 2, 2023
@adela-almasan adela-almasan marked this pull request as ready for review October 2, 2023 20:00
@adela-almasan adela-almasan requested a review from a team as a code owner October 2, 2023 20:00
@adela-almasan adela-almasan requested review from ashharrison90 and L-M-K-B and removed request for a team October 2, 2023 20:00
Copy link
Contributor

@nmarrs nmarrs left a comment

Choose a reason for hiding this comment

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

LGTM

@nmarrs nmarrs added the area/grafana/ui Issues that belong to components in the @grafana/ui library label Oct 2, 2023
Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

don't think we would want to do this via an effect. have started a discussion in slack to figure out the best way of handling this 👍

Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

so looking at the documentation for toggletip i think we'd recommend against putting this kind of stuff in a Toggletip altogether and probably recommend a Modal instead. one of the main don'ts for Toggletip is:

Do not use to surface actions to users

if we were to add something like this, which i think does have value, i think we'd want to do it in the form of a show prop similar to Tooltip whereby you manually handle the visible state outside of the Toggletip component.

@staton-hyse11
Copy link
Contributor

This seems like a misuse of the toggletip component. The use case shows a user having a dialogue with the system, which would point to a modal being more appropriate. Toggletips should not be vehicles for users to configure, edit, or select data on a page. They are primarily used for providing contextual information and/or linking to documentation that will help users gain more information about their task at hand, not really for managing a task themself — if that makes sense. I'm happy to chat through this more if you want.

@ivanortegaalba
Copy link
Contributor

ivanortegaalba commented Oct 3, 2023

The usage of toogletip here is to take references of leaders in generative AI in the market. Here is an example of Grammarly:
Captura de pantalla 2023-10-03 a las 17 32 10

This "popover" appears to assist you in generating something on a specific input. Opening a modal completely removes the user from the context, and disturbs a lot of the experience. Actually, you create an experience itself about improving a text, where the main task for the user is to autogenerate and refine the result with some feedback.

image

Do you think, for this use case, the best UX is a modal? Can't we use a floating component that can be reused in any input field that can be auto-generated? Is here a modal a better experience? If it is, why?

@torkelo
Copy link
Member

torkelo commented Oct 3, 2023

If we are to use this component with forms inside it's a popover and we should use the popover background (primary) like all our other popovers

@torkelo
Copy link
Member

torkelo commented Oct 3, 2023

For popover reference, TimeRangePicker, ColorPickerPopover can't think of others right now.

@staton-hyse11
Copy link
Contributor

After discussing with @ivanortegaalba, this use case makes sense for the toggletip.

We'll look to create a generic popover component in the future to help with use-cases like this and align with the proper background color and as the other popovers use.

Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

if we were to add something like this, which i think does have value, i think we'd want to do it in the form of a show prop similar to Tooltip whereby you manually handle the visible state outside of the Toggletip component.

this comment still applies ☝️

@ivanortegaalba
Copy link
Contributor

if we were to add something like this, which i think does have value, i think we'd want to do it in the form of a show prop similar to Tooltip whereby you manually handle the visible state outside of the Toggletip component.

Agree! I can apply that feedback if that's ok @adela-almasan

@ivanortegaalba
Copy link
Contributor

I pushed an update with the new prop @ashharrison90. It should be fine now

@nmarrs
Copy link
Contributor

nmarrs commented Oct 5, 2023

@ashharrison90 given recent changes / addressing the show prop feedback are you comfortable removing (or with us removing) your request for changes on this PR? Thanks!

Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@ivanortegaalba ivanortegaalba merged commit 586c78a into main Oct 5, 2023
17 checks passed
@ivanortegaalba ivanortegaalba deleted the toggletip_shouldClose branch October 5, 2023 06:58
@torkelo
Copy link
Member

torkelo commented Oct 5, 2023

are we changing the design (background) in a follow-up PR?

@adela-almasan
Copy link
Contributor Author

@torkelo yes - first step here #76069. I've just added a "custom" theme with the styling we want (primary background) - I think it's less invasive than allowing custom styling and I'm not sure if that's something we want to support right now. cc @ashharrison90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend area/grafana/ui Issues that belong to components in the @grafana/ui library no-backport Skip backport of PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

7 participants