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

[OSCI][BUG] Fixing bug in icon-only buttons for showing info tooltip when hovered #1160

Merged
merged 9 commits into from
Jan 11, 2024

Conversation

BigSamu
Copy link
Contributor

@BigSamu BigSamu commented Nov 17, 2023

Description

Icon-only buttons are not showing descriptive tooltip when hovered. This PR fixes the problem by adding wrapping a tag with textProps to a <buttonIcon/> component

Issues Resolved

Issue #1129

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • All tests pass
    • yarn lint
    • yarn test-unit
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@BigSamu BigSamu changed the title [OSCI]{BUG] Fixing bug in icon-only buttons for showing info tooltip when hovered [OSCI][BUG] Fixing bug in icon-only buttons for showing info tooltip when hovered Nov 17, 2023
@BSFishy BSFishy added the OSCI label Dec 4, 2023
… undefined in className instead of empty string)

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>
@BigSamu
Copy link
Contributor Author

BigSamu commented Dec 7, 2023

@BSFishy, @joshuarrrr,

I found a problem with the documentation page of the button component, which is not triggered by my change. If you try to review the props for an OuiButton element, you will see the following error below:

image

Is there any issue with this? Do you know what could be causing this?

Also, I realized that for the case of the examples of many buttons (not group buttons), textProps are not being provided. Should we update these examples so that when you hover you get also the showing info tooltip?

Regards,

Samuel

@BSFishy
Copy link
Contributor

BSFishy commented Dec 7, 2023

Is there any issue with this? Do you know what could be causing this?

This seems to be a bigger issue. I did a little bit of digging and found that the docinfo isn't being generated for OuiButton for some reason. I'll make an issue for this.

#1169

@BigSamu
Copy link
Contributor Author

BigSamu commented Jan 1, 2024

@BSFishy, @joshuarrrr,

Last updates requested are done. I added comments in a code review above so you can check the exact changes I made. I am still a bit confused about how to use snapshots. But I would bring that question to the following community office hours.

Regards,

Samuel

@BigSamu BigSamu requested a review from joshuarrrr January 11, 2024 20:33
@BigSamu
Copy link
Contributor Author

BigSamu commented Jan 11, 2024

@joshuarrrr,

Revies asked are resolved. Could you check for proceeding with a merge?

@joshuarrrr joshuarrrr merged commit 96af93e into opensearch-project:main Jan 11, 2024
17 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 11, 2024
…when hovered (#1160)

* fixing bug in icon-only buttons for showing info tooltip

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

* Updating test snapshoots related to change in button_content.tsx

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

* Updating chanelog.md regarding update in OuiButtonContent

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

* updating button_content.tsx using recomendation of mantainer (include undefined in className instead of empty string)

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

* updating test snapshoots for button_content.tsx

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

* adding title prop to OuiIcon component, removing wrapping span element in buttonIcon and updating snapshots

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

* refactoring code for buttonIcon variable to keep more concise definition

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

---------

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>
(cherry picked from commit 96af93e)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
@BigSamu BigSamu deleted the bug/hover-icon-button branch January 12, 2024 12:35
BigSamu added a commit to BigSamu/oui that referenced this pull request Jan 13, 2024
AMoo-Miki pushed a commit that referenced this pull request Jan 25, 2024
…when hovered (#1160) (#1206)

* fixing bug in icon-only buttons for showing info tooltip

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

* Updating test snapshoots related to change in button_content.tsx

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

* Updating chanelog.md regarding update in OuiButtonContent

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

* updating button_content.tsx using recomendation of mantainer (include undefined in className instead of empty string)

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

* updating test snapshoots for button_content.tsx

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

* adding title prop to OuiIcon component, removing wrapping span element in buttonIcon and updating snapshots

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

* refactoring code for buttonIcon variable to keep more concise definition

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

---------

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>
(cherry picked from commit 96af93e)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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