Skip to content

Infotip - Add Icon tab#50

Merged
nagpai merged 31 commits intomainfrom
feature/22-icon-tab
Jun 21, 2025
Merged

Infotip - Add Icon tab#50
nagpai merged 31 commits intomainfrom
feature/22-icon-tab

Conversation

@nagpai
Copy link
Contributor

@nagpai nagpai commented Jun 10, 2025

Summary

  • Add a tab with controls to add an icon beside content that has Infotip format applied.
  • Includes choice among 6 preset icons, direction of placement and icon color.

Related Issue

Closes #22

Testing instructions

  • Check the code changes for any errors or improvements
  • Try adding an infotip format, and using various options within the Icon tab. The tab gets enabled only if there is content added.

@nagpai nagpai requested a review from Copilot June 10, 2025 16:42
@github-actions
Copy link

github-actions bot commented Jun 10, 2025

Preview via Cloudflare R2 Storage

⚡️WordPress Playground Preview
🚀 Build zip file Download

I will update this comment with the latest preview links as you push more changes to this PR.

Note

The preview sites are created using WordPress Playground. You can add content, edit settings, and test the pull request as you would on a real site, but please note that changes are not saved between sessions.

This comment was marked as outdated.

@nagpai nagpai changed the title WIP - Icon tab Infotip - Add Icon tab Jun 14, 2025
@nagpai nagpai requested review from ajitbohra and Copilot June 14, 2025 13:50
@nagpai nagpai added the pr:needs-review PR ready for review label Jun 14, 2025
@nagpai nagpai marked this pull request as ready for review June 14, 2025 13:51

This comment was marked as outdated.

@Verma-Punit
Copy link
Contributor

@nagpai I have tested a part of your PR and noticed something that might be a bug though I am not completely sure. In the video I shared, you will see that when I add text in the infotip, it works perfectly. However, when I remove the text, the infotip still appears empty unless I manually click the clear button. Ideally, I feel the clear button should be disabled when there is no text from a UX perspective, that might make more sense.

Screen.Recording.2025-06-15.at.12.34.05.PM.mov

Copy link
Contributor

@Verma-Punit Verma-Punit left a comment

Choose a reason for hiding this comment

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

@nagpai When I enable the icon, the default icon button does not appear selected. Also, when I hover over the color option, it changes to blue, which seems off. Ideally, this should not happen.

Screen.Recording.2025-06-15.at.12.56.23.PM.mov

Copy link
Member

@ajitbohra ajitbohra left a comment

Choose a reason for hiding this comment

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

code wise looks good.

tested and editor UI observed tooltip overlaps the content lets align this with the frontend preview.

image

@ajitbohra
Copy link
Member

@nagpai PR looks good and solid few pointers:

  • For now, let's ship the disabled tab as it is and explore better handling if the disabled settings separately
  • Let's fix the tooltip overlap in the editor
  • Let's fix the default color issue as mentioned by Punit
  • The empty tooltip spotted by Punit is interesting We can handle that as separate issue as it looks tricky

@nagpai
Copy link
Contributor Author

nagpai commented Jun 16, 2025

@nagpai When I enable the icon, the default icon button does not appear selected. Also, when I hover over the color option, it changes to blue, which seems off. Ideally, this should not happen.

@Verma-Punit For now I have changed the label of the color to Custom color that shows as not set. The icon uses the text color as default in its render. The hover change was happening since I had used customColor as fallback. Trying to find out what is the best way to get the default text color ( as defined in Styles within site editor ) as a param to pass as default for the color setting.

Verma-Punit and others added 7 commits June 18, 2025 19:35
Make sure ShadowDOM is rendered before going ahead with rest of the code to ensure the first character of infotip is visible while creating it in editor.
@nagpai
Copy link
Contributor Author

nagpai commented Jun 20, 2025

Status

For now, let's ship the disabled tab as it is and explore better handling if the disabled settings separately 🟡

Status quo on this one. The tab will remain or turn disabled when content is blank.

Let's fix the tooltip overlap in the editor 🔴

This seems to be a blocker to get most of the changes merged, and would be better to focus on this separately. I will create a separate issue to investigate deeper and fix this. ( Update: Issue creatd here - #53 )

This could relate to the boundaries of the parent element not being read correctly when we create a new tooltip. However, when we save and reload, the offset appears correctly.

Let's fix the default color issue as mentioned by Punit - ✅

Props to @Verma-Punit for fixing this!

The empty tooltip spotted by Punit is interesting We can handle that as separate issue as it looks tricky - ✅

This has been fixed in the current PR! 🎉

Re-requesting review from @ajitbohra and @Verma-Punit 🙏🏼

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new “Icon” tab to the Infotip inline UI, letting users choose from preset icons, set placement direction, and pick icon color.

  • Introduces IconTabContent component with enable toggle, icon selection, position controls, and color settings
  • Extends infotip format attributes (icon-enabled, icon-type, icon-position, icon-color)
  • Updates web component to render and style the selected icon alongside the infotip text

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
src/infotip/inline-ui.js Added IconTabContent, wired up controls in InlineUI
src/infotip/index.js Added new format attribute keys for icon settings
src/infotip/editor.scss Updated editor styles for the Icon tab (labels, layout spacing)
assets/infotip/infotip-web-component.js Web component enhancements to render icon based on attributes

const sanitizedValue = safeHTML( newValue );

// if the value is empty remove the format
if ( ! sanitizedValue ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@nagpai We can remove this condition since it remove the formatting, and you have already handled the empty tooltip text in the web component. That should take care of the issue. 😊

Copy link
Contributor Author

@nagpai nagpai Jun 20, 2025

Choose a reason for hiding this comment

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

Removing this can leave a format without a tooltip lingering, hence was wondering if we should keep it. What do you say?

To elaborate - The formatted text looks like this, without a tooltip, if I add some content and empty it. -
image

The user can certainly mark the text again and click the clear button, but I thought it would be better experience if the format gets removed immediately if they blank the content. It gets added again, if they add text once more.

It can also be confusing, if the user disables the underline. There will be the format markup left behind in the code ( nothing too breaking there, but unwanted code )

Copy link
Contributor

Choose a reason for hiding this comment

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

@nagpai Your point absolutely makes sense but I think the underline can be manually disabled, so in this case, this might have just been a small miss where the text was empty, but underline stayed on.

Also, consider this second scenario: if a user adds an infotip format to some text, enables the icon, and later wants to change the text they would lose all their settings just by removing and retyping the text.

Another possible case: the user sets up an infotip with text and icon, but later removes the text and saves it without removing the format which could also lead to confusion.

Maybe I m overthinking this 😅 @ajitbohra, would love your thoughts too!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rehearsed this again by removing the code, and going by the scenarios you mentioned, @Verma-Punit, it may be better to let the user click Clear explicitly to remove the format ( by removing that extra conditional ) .

Here is what I have done now:

  1. The extra conditional is removed.
  2. If user makes the text blank, it no longer removes the format. Rather allows change of the overlay text
  3. The icon tab remains enabled as long as there is an icon enabled. Preserves the icon style until explicitly removed or changed.

Maybe I m overthinking this 😅 @ajitbohra, would love your thoughts too!

You are not overthinking at all. Good to have different perspectives of user experience and then starting off with the best possibility! Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes here - c64419d ..

Let me know how it works now @Verma-Punit .

Copy link
Contributor

Choose a reason for hiding this comment

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

@nagpai working perfectly ✨🚀 Thank You! 🙂

@Verma-Punit
Copy link
Contributor

@nagpai While testing, I came across another issue. When I apply the Infotip format, I am unable to add any text after it. I have shared a short video to show the issue after formatting the word world in hello world I tried adding more text, but it does not let me type anything beyond that point.

Screen.Recording.2025-06-21.at.7.45.07.AM.mov

Prevents losing custom icon settings, if a user changes all text ( making it blank first )
@nagpai
Copy link
Contributor Author

nagpai commented Jun 21, 2025

@nagpai While testing, I came across another issue. When I apply the Infotip format, I am unable to add any text after it. I have shared a short video to show the issue after formatting the word world in hello world I tried adding more text, but it does not let me type anything beyond that point.

I tried this a few times and it is indeed a bit jittery. It works if I click on the anchor text, or very close to it. I am not seeing any errors in the console, but it could be related to how the text is enclosed within shadowDOM (?) - Guessing here. I will check to see if there is a way to fix this easily.

infotip-text-demo-1

@ajitbohra
Copy link
Member

Regarding the case where we remove the text and format, it will result in a poor user experience, as the user simply wants to rewrite the text, and we remove it. What we can do here is instead have a fallback text, kindly add content or clear format, which will help with all edge cases and be a fail-safe.

Unable to reproduce the text edit issue mentioned by Punit.

For icon setting, how about showing the setting only when it is enabled and hiding controls by default?

image

Not focusing much on the code, it can undergo some refactoring, but let's ship the implementation and later focus on the code.

@nagpai we can merge this as the foundation and keep iterating and fine-tune as separate PR's to keep things moving.

@nagpai
Copy link
Contributor Author

nagpai commented Jun 21, 2025

Thanks @ajitbohra 🙌🏼 .

Regarding the case where we remove the text and format, it will result in a poor user experience, as the user simply wants to rewrite the text, and we remove it. What we can do here is instead have a fallback text, kindly add content or clear format, which will help with all edge cases and be a fail-safe.

This is a great approach. I have already gone ahead with not removing the format. I have added a placeholder text that shows up if the content text area is empty.

For icon setting, how about showing the setting only when it is enabled and hiding controls by default?

I will create a separate PR for this iteration. I am wanting to check layout shift and best way to accommodate this. I hope that is fine. ( Update - Created an Issue to track this #54 )

I will go ahead and merge the rest of the changes now.

@nagpai nagpai merged commit f4da92b into main Jun 21, 2025
2 checks passed
@nagpai nagpai deleted the feature/22-icon-tab branch June 21, 2025 12:40
@nagpai nagpai removed the pr:needs-review PR ready for review label Jun 21, 2025
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.

Infotip: add icon for info tip around text

4 participants