Use tooltip icons to show tooltips rather than label/input elements#2130
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve updates to the email settings form, CSS styles, and JavaScript functionality within a form builder. The email settings form has new anchor elements for removing email addresses in the CC and BCC sections. The CSS file has been extensively modified to introduce new variables and styles, improving the overall UI. Additionally, the JavaScript file has enhancements for error handling, visibility toggling, and improved user feedback in the form builder's interface. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
classes/views/frm-form-actions/_email_settings.php (1)
Line range hint
23-73: Maintain consistent UI patterns across form elementsThe Reply To field implementation (around line 64) establishes a clear pattern:
- Single remove icon in the label
- Uses
frm_cancel1_iconclass- Proper
data-emailrowattributeThe CC and BCC fields should follow this same pattern for consistency and maintainability.
Consider standardizing all email field remove icons to:
- Always appear in the label
- Always use the
frm_cancel1_iconclass- Have correct
data-emailrowattributes matching their respective fieldscss/frm_admin.css (1)
5832-5836: Consider Using CSS Variables for Consistent SpacingThe hardcoded value
20pxin thewidthcalculation could be replaced with a CSS variable to enhance consistency and maintainability. This aligns with the existing use of spacing variables in the stylesheet.You might define a new variable if one doesn't already exist:
--gap-base: 20px;Then update the code:
-.frm_cc_row.frm_email_row .frm-with-right-icon, -.frm_bcc_row.frm_email_row .frm-with-right-icon { - width: calc(100% - 20px); + width: calc(100% - var(--gap-base)); }js/formidable_admin.js (1)
8573-8576: Refactor suggestion: Use a switch statement instead of multiple if statements.Instead of using multiple if statements to check the value of
useTagHref, consider using a switch statement for better readability and performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
classes/views/frm-form-actions/_email_settings.php(2 hunks)css/frm_admin.css(1 hunks)js/formidable_admin.js(1 hunks)
🔇 Additional comments (1)
css/frm_admin.css (1)
5838-5840: Consistent Use of CSS Variables Enhances Maintainability
Setting the fill property using var(--grey-500) promotes consistency across the stylesheet and makes future theming adjustments easier.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2130 +/- ##
============================================
- Coverage 27.17% 27.16% -0.01%
- Complexity 8158 8170 +12
============================================
Files 127 127
Lines 27104 27143 +39
============================================
+ Hits 7365 7374 +9
- Misses 19739 19769 +30 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
@AbdiTolesa Should these be the tooltip icon (i icon)? 
|
@truongwp That's what Razvan's design look in https://github.com/Strategy11/formidable-pro/issues/5103#issuecomment-2488774002 |
|
@AbdiTolesa Look a bit closer at Razvan's design and the conversation. That icon should be the tooltip icon now. |
@truongwp Sorry for the oversight! The icons indeed should be tooltip icons and that is changed now. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
css/frm_admin.css (1)
5838-5842: Consider using CSS custom properties for icon colorsThe icon styling looks good but could be improved by using CSS custom properties for colors to maintain consistency.
.frm_email_row .frm_remove_field .frm_close_icon { position: relative; - fill: var(--grey-500); + fill: var(--icon-color, var(--grey-500)); vertical-align: middle; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
css/frm_admin.css(2 hunks)js/formidable_admin.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- js/formidable_admin.js
🔇 Additional comments (1)
css/frm_admin.css (1)
5832-5836: LGTM! Width calculation for email fields
The width calculation ensures proper spacing for the remove icon while maintaining a clean layout.
There was a problem hiding this comment.
@AbdiTolesa This issue isn't totally fixed.
- I still see a tooltip when hovering over the "To", "From", and "Subject" labels (and inputs). There is no tooltip icon in this case. We want to stop showing tooltips on labels/inputs, and instead use tooltip icons.
- The tooltips should only appear when hovering over the tooltip icon. The main goal of this issue is to remove the tooltip on the whole label spanning the full width. We don't want the tooltips to show up everywhere.
- It looks like you missed "Reply To". It should be the same as CC/BCC.
- I think we should do this everywhere we show a tooltip without an icon. I see "Action Name" also does this. When I hover it, I see a double tooltip issue coming from the input as well right now.
AbdiTolesa
changed the title
@Crabcyborg Sorry I thought this update should only over cc/bcc fields and primarily be focused on the remove icon for those fields. |
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
Thanks @AbdiTolesa!
This looks close now. I still see one issue though.
The reply-to icon doesn't have any X icon like the CC/BCC do. Since these are hidden by default and can be toggled on, all three of these should work the same.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
css/frm_admin.css (1)
5832-5837: Consider using CSS custom properties for width calculationThe width calculation could be made more maintainable by using CSS custom properties.
-.frm_cc_row.frm_email_row .frm-with-right-icon, -.frm_bcc_row.frm_email_row .frm-with-right-icon, -.frm_reply_to_row.frm_email_row .frm-with-right-icon { - width: calc(100% - 20px); - display: inline-block; -} +.frm_cc_row.frm_email_row .frm-with-right-icon, +.frm_bcc_row.frm_email_row .frm-with-right-icon, +.frm_reply_to_row.frm_email_row .frm-with-right-icon { + --icon-width: 20px; + width: calc(100% - var(--icon-width)); + display: inline-block; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
classes/views/frm-form-actions/_email_settings.php(2 hunks)css/frm_admin.css(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/views/frm-form-actions/_email_settings.php
🔇 Additional comments (1)
css/frm_admin.css (1)
5839-5844: LGTM! Clean styling for the remove icon
The styling for the remove field icon is clean and follows the existing color scheme using the grey-500 variable.
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
I just fixed an issue.
I think this is good to merge now.
Thank you Abdi and Laura for working on this!
🚀
Crabcyborg
deleted the
issue-5103-add_additional_remove_icon_for_cc_bcc_fields
branch
4 participants
Fix https://github.com/Strategy11/formidable-pro/issues/5103
Quoting the third item in the checklist from the issue:
Design reference:
https://github.com/Strategy11/formidable-pro/issues/5103#issuecomment-2488774002