-
Notifications
You must be signed in to change notification settings - Fork 67
fix: colors of page icons not being shown/applied #3665
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
Conversation
WalkthroughAdds a guard to skip page-icon processing when an icon is multicolor and switches page-icon hiding from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Pull request artifacts
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/plugins/page-icons/index.js (1)
31-32: LGTM! Offscreen positioning preserves gradient rendering.The switch from
display: noneto offscreen positioning correctly addresses the gradient rendering issue. The technique is standard and appropriate for keeping SVG elements in the rendering tree while visually hiding them.For better maintainability, consider extracting the inline style to a CSS class:
/* In your stylesheet */ .stk-page-icons-hidden { position: absolute; top: 0; left: -9999px; width: 0; height: 0; visibility: hidden; }Then update the code:
- // Don't use `display: none` to hide the page icons because it prevents gradients from being applied. - pageIconsWrapper.setAttribute( 'style', 'position: absolute; top: 0; left: -9999px; width: 0; height: 0; visibility: hidden;' ) + // Don't use `display: none` to hide the page icons because it prevents gradients from being applied. + pageIconsWrapper.setAttribute( 'class', 'stk-page-icons-hidden' )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/block-components/icon/index.js(1 hunks)src/plugins/page-icons/index.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/block-components/icon/index.js (2)
src/block-components/typography/index.js (1)
getAttribute(147-150)src/components/block-css/util.js (1)
getAttribute(209-230)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: build
| // Don't use page icons for multicolor icons | ||
| // because we target svg elements with the :nth-of-type() selector to apply the multicolor styles. | ||
| if ( getAttribute( 'iconColorType' ) === 'multicolor' ) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add cleanup when iconColorType changes to/from multicolor.
The early return prevents page-icon processing for multicolor icons. However, if iconColorType changes from a regular type to multicolor during the icon's lifecycle, the icon will remain in the page-icons store without being removed. Similarly, changing from multicolor to a regular type won't trigger page-icon processing.
Consider adding getAttribute( 'iconColorType' ) to the useEffect dependency array and handling the transition:
- }, [ _icon ] )
+ }, [ _icon, getAttribute( 'iconColorType' ) ] )Additionally, add cleanup logic before the early return:
// Don't use page icons for multicolor icons
// because we target svg elements with the :nth-of-type() selector to apply the multicolor styles.
if ( getAttribute( 'iconColorType' ) === 'multicolor' ) {
+ // Clean up if this icon was previously in the page-icons store
+ if ( processedIconRef.current === _icon && _icon ) {
+ dispatch( 'stackable/page-icons' ).removePageIcon( _icon )
+ processedIconRef.current = null
+ setIcon( _icon ) // Use the original icon directly
+ lastIconValueRef.current = _icon
+ }
return
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/block-components/icon/index.js around lines 205 to 209, the effect
currently returns early for iconColorType === 'multicolor' but doesn't clean up
or respond to transitions; add getAttribute('iconColorType') to the useEffect
dependency array and inside the effect detect transitions: when switching to
'multicolor' call the existing cleanup/remove-from-page-icons logic before
returning, and when switching from 'multicolor' to a regular type ensure you
call the page-icon processing/registration routine; ensure the cleanup function
is invoked on unmount or before early returns so page-icons store stays
consistent.
Summary by CodeRabbit
Bug Fixes
Behavior
Accessibility & Performance
✏️ Tip: You can customize this high-level summary in your review settings.