Stop using font icon in section field icons in form builder#2146
Conversation
AbdiTolesa
changed the title
AbdiTolesa
changed the title
|
Warning Rate limit exceeded@AbdiTolesa has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces changes to the rendering logic and styling of form fields in the Formidable Forms admin interface. In Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (2)
classes/views/frm-forms/add_field.php (1)
Line range hint
7-103: Consider implementing a consistent icon systemGiven the PR's objective to move away from font icons, consider implementing a more systematic approach:
- Create a centralized icon component/helper that supports multiple icon types (SVG, font, etc.)
- This would make it easier to transition away from font icons gradually
- Consider adding a configuration option to control the icon implementation type
Example approach:
// Icon helper that supports multiple types FrmIconHelper::render('form-title-style', [ 'type' => 'svg', // or 'font' for backward compatibility 'class' => 'your-classes' ]);Would you like me to help draft a detailed proposal for this architectural improvement?
css/frm_admin.css (1)
6579-6587: Consider removing the redundantcolorpropertyIn the selector
.edit_field_type_divider .frmsvg, bothfillandcolorproperties are specified. Sincefillis used to set the color of SVG content, thecolorproperty might be unnecessary unless it's intended for fallback or affecting non-SVG child elements. Removing redundant properties can simplify the code and improve maintainability.Apply this diff to remove the unnecessary
colorproperty:.edit_field_type_divider .frmsvg { padding: 2px; position: absolute; left: -12px; fill: var(--primary-700); - color: #fff; background-color: var(--primary-700); border-radius: 50%; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
classes/views/frm-forms/add_field.php(2 hunks)css/frm_admin.css(1 hunks)
🔇 Additional comments (2)
classes/views/frm-forms/add_field.php (2)
103-103: Review implementation approach for icon replacement
Similar to the previous icon, this adds another font icon (frmfont frm-circle), which appears to contradict the goal of removing font icons from section fields.
Consider:
- Should these be replaced with SVG icons or another non-font icon approach?
- Is there a transition plan to move away from font icons?
Let's check for any existing SVG icon implementations that could be used instead:
7-9: Verify alignment with PR objectives
The addition of FrmAppHelper::icon_by_class('frmfont frm-form-title-style') appears to introduce a new font icon, which seems to contradict the PR objective of stopping the use of font icons in section field icons.
Let's check if there are any related icon implementations that might provide insight into the intended approach:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
css/frm_admin.css (1)
6589-6597: Consider using CSS custom properties for dimensionsWhile the styles for the start divider icon are functionally correct, consider extracting the hardcoded dimensions (8px, -4px, -24.5px) into CSS custom properties for better maintainability and consistency.
.edit_field_type_divider .start_divider .frmsvg { - height: 8px; - width: 8px; + height: var(--divider-icon-size, 8px); + width: var(--divider-icon-size, 8px); padding: 0; - bottom: -4px; - left: -24.5px; + bottom: var(--divider-icon-offset-bottom, -4px); + left: var(--divider-icon-offset-left, -24.5px); color: var(--primary-700); background-color: #fff; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
css/frm_admin.css(1 hunks)
🔇 Additional comments (2)
css/frm_admin.css (2)
6579-6587: LGTM: Base SVG icon styles for section dividers
The base styles for section divider icons are well-structured and use CSS custom properties consistently for colors. The absolute positioning and circular background create a clean visual indicator.
6598-6606: LGTM: Hover and selected states
The hover and selected states are well implemented, using the established color system and maintaining visual consistency with the interface.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
css/frm_admin.css (1)
6598-6606: Add transition for smoother hover effectsThe hover state changes could benefit from a smooth transition effect.
Apply this diff:
.edit_field_type_divider.selected .frmsvg, .edit_field_type_divider:hover .frmsvg { background: var(--primary-500); + transition: background-color 0.2s ease-out; } .edit_field_type_divider.selected .start_divider .frmsvg, .edit_field_type_divider:hover .start_divider .frmsvg { background-color: #fff; - color: var(--primary-500); + fill: var(--primary-500); + transition: fill 0.2s ease-out; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
css/frm_admin.css(1 hunks)
🔇 Additional comments (2)
css/frm_admin.css (2)
6588-6596: 🛠️ Refactor suggestion
Previous suggestion for using fill still applies
The same issue exists in this selector where color is used instead of fill for SVG styling.
Apply this diff:
.edit_field_type_divider .start_divider .frmsvg {
height: 8px;
width: 8px;
padding: 0;
bottom: -4px;
left: -24.5px;
- color: var(--primary-700);
+ fill: var(--primary-700);
background-color: #fff;
}6579-6587: 🛠️ Refactor suggestion
Consider using fill instead of color for SVG icons
For SVG elements, the fill property is more appropriate than color for controlling the icon color. This ensures consistent rendering across browsers.
Apply this diff:
.edit_field_type_divider .frmsvg {
padding: 2px;
position: absolute;
left: -12px;
- color: #fff;
+ fill: #fff;
background-color: var(--primary-700);
border-radius: 50%;
}Likely invalid or redundant comment.
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
@AbdiTolesa It looks like this update causes a lot of issues with other icons inside of sections. The move field icon, the more options dropdown, the option in that dropdown, etc.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
css/frm_admin.css (1)
6588-6596: Consider using CSS custom properties for magic numbersWhile the styles are generally good, consider extracting magic numbers like
8px,4px,-24.5pxinto CSS custom properties for better maintainability..edit_field_type_divider .start_divider .frm-divider-icon.frmsvg { - height: 8px; - width: 8px; + height: var(--divider-icon-size, 8px); + width: var(--divider-icon-size, 8px); padding: 0; bottom: -4px; - left: -24.5px; + left: var(--divider-icon-offset, -24.5px); color: var(--primary-700); background-color: #fff; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
classes/views/frm-forms/add_field.php(2 hunks)css/frm_admin.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/views/frm-forms/add_field.php
🔇 Additional comments (2)
css/frm_admin.css (2)
6579-6587: LGTM: Base divider icon styles are well structured
The base styles for the divider icon are well implemented with:
- Proper use of CSS variables for colors and consistent theming
- Good positioning with absolute positioning
- Appropriate use of transitions for hover states
6598-6606: LGTM: Hover and selected states are properly handled
The hover and selected states are well implemented:
- Consistent use of CSS variables for colors
- Proper specificity in selectors
- Good separation of concerns between states
Crabcyborg
deleted the
issue-5504-stop_using_font_icon_in_section_field_icon
branch
|
@AbdiTolesa It looks like I merged a bit early. I forgot that this update would also impact repeaters. Can you get it looking like it did before? Now it shows the H, and the repeater icon is floating outside of the circle. Before this update After this update Thank you! |
2 participants
Fix https://github.com/Strategy11/formidable-pro/issues/5504
