opensearch-project / oui

OpenSearch UI Framework
Apache License 2.0
33 stars 69 forks source link

Hover over the filter is not showing text in vertical bar visualization in OpenSearch dashboards #1129

Closed tejashu7 closed 8 months ago

tejashu7 commented 1 year ago

Description: Hover over the filter is not showing text in vertical bar visualization in OpenSearch dashboars.

Steps to reproduce the behavior:

  1. Create a vertical dashboard with 2 legends.
  2. Click on the legend it will open the color palette and filter options.
  3. Hover on the “+” → filter out value which appears after the legend is clicked for color palette filter as shown for Kibana. The hover text over that “+/-” sign is not appearing for OpenSearch-dashboards.

Expected behavior A text descipbing the filter out value should be shown when hovered over the "+" sign.

OpenSearch Version OpenSearch 2.6.0

Dashboards Version OpenSearch Dashboard version is 2.6.0

Plugins

Please list all plugins currently enabled.

Screenshots

If applicable, add screenshots to help explain your problem.

Host/Environment (please complete the following information):

Additional context

Please find the forum link for further details: https://forum.opensearch.org/t/hover-over-filter-in-vertical-bar-in-opensearch-dashboards-not-showing-text/14124

joshuarrrr commented 1 year ago

Thanks @tejashu7!

tejashu7 commented 10 months ago

Hi,

Any updates on this issue?

Thanks, Tejas

joshuarrrr commented 10 months ago

I took a bit of a look and it turns out this is actually an issue in the underlying OUI component, <EuiButtonGroup>: https://oui.opensearch.org/1.0/#/navigation/button#button-groups

The problem is with the implementation of the isIconOnly prop. Before we go about fixing it, @kgcreative can you confirm that we should still have visible labels for icon-only buttons (labels are actually essential when only using an icon to better hint what the icon represents).

BigSamu commented 10 months ago

@joshuarrrr,

May I take a look at this issue? Feeling is something straightforward or not?

Regards,

Samuel

joshuarrrr commented 10 months ago

Yep, @BigSamu , I think this would be a good issue for you to do a little more investigation on. We'll still need Kevin to provide some guidance on the intended behavior, but you can dig into the implementation of the isIconOnly prop to see how we might best fix it.

kgcreative commented 10 months ago

@joshuarrrr + @BigSamu -- For accessibility (and usability) reasons, if the form element doesn't have a label (which looks like button groups probably wouldn't), then a tooltip (https://oui.opensearch.org/1.3/#/display/tooltip) should display that information. filter for and filter out make sense here, although a nice to have would be filter for {key:value}

BigSamu commented 10 months ago

Yep, @BigSamu , I think this would be a good issue for you to do a little more investigation on. We'll still need Kevin to provide some guidance on the intended behavior, but you can dig into the implementation of the isIconOnly prop to see how we might best fix it.

Great! Thanks @joshuarrrr !

@joshuarrrr + @BigSamu -- For accessibility (and usability) reasons, if the form element doesn't have a label (which looks like button groups probably wouldn't), then a tooltip (https://oui.opensearch.org/1.3/#/display/tooltip) should display that information. filter for and filter out make sense here, although a nice to have would be filter for {key:value}

Perfect, thanks @kgcreative, I will start working on this issue next week. Now much more familiarized with the whole OUI base code.

BigSamu commented 10 months ago

@joshuarrrr @kgcreative

It took me a while to understand the whole implementation of both codes (osd and oui) to understand the problem. But at last, here I have a simple solution.

In OUI library, in button_content.tsx file, we can have the following for the return of the component:

 return (
    <span {...contentProps} className={contentClassNames}>
      <span {...textProps} className="" >{buttonIcon} </span>
      <span {...textProps}>{children}</span>
    </span>
  );

Instead of,

return (
    <span {...contentProps} className={contentClassNames}>
      {buttonIcon}
      <span {...textProps}>{children}</span>
    </span>
  );

The dictionary textProps for the case of a iconOnly button contains the following:

{className: 'ouiButton__text ouiButtonGroupButton__textShift', data-text: 'Option three', title: 'Option three', ref: ƒ}

By adding, className="" after {...textProps} we override the initial definition of className coming in textProps. This simple solution works allowing us to visualize the tooltip. Of course, I feel it is not quite elegant, and maybe we should move the HTML attributes data-text and title to the contentProps

Below is a screenshot applying this small change in the code of the OUI documentation.

image

Looking forward to your comments.

Samuel

joshuarrrr commented 10 months ago

@BigSamu I think that makes sense - we may need to validate that duplicating the props don't cause any side effects, but go ahead and open a PR, and we can go from there.

BigSamu commented 10 months ago

Hi @joshuarrrr @kgcreative, PR #1160 is already open for your review. Looking forward to your comments.

joshuarrrr commented 8 months ago

completed via #1160. For follow-up, see https://github.com/opensearch-project/oui/issues/1207

BigSamu commented 8 months ago

I will take #1207 to continue enhancing this feature