-
Notifications
You must be signed in to change notification settings - Fork 1
Review fixes #11
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
base: main
Are you sure you want to change the base?
Review fixes #11
Conversation
|
Test on Playground |
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.
Pull Request Overview
This PR updates the plugin's text domain from 'color-scheme' to 'brand-assets' and refactors the settings page to use structured helper methods, replacing the custom CSS option with a comprehensive set of popover styling controls.
- Updated all internationalization strings to use the correct 'brand-assets' text domain
- Refactored settings page to use reusable rendering methods for form fields
- Replaced the "custom CSS" loading mode with individual popover styling options using CSS custom properties
- Extracted inline JavaScript to separate files for better maintainability
- Changed pattern file from
.incto.phpand updated loading mechanism
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/edit.js | Updated text domain from 'color-scheme' to 'brand-assets' and added eslint-disable comments for console.log statements |
| readme.txt | Updated documentation to remove custom CSS option and reference new filter-based customization approach |
| phpcs.xml | Excluded long closure rules to accommodate refactored code |
| package-lock.json | Updated dependency versions (caniuse-lite, fsevents) |
| includes/class-brand-assets.php | Changed pattern file extension from .inc to .php and updated file loading from file_get_contents to require |
| includes/class-brand-assets-settings.php | Major refactor: added helper methods for rendering form fields, replaced custom CSS with structured styling options, and extracted JavaScript to external file |
| includes/class-brand-assets-options.php | Added default values for popover styling options and removed 'custom' CSS mode |
| includes/class-brand-assets-frontend.php | Refactored CSS loading to generate CSS custom properties from user settings and extracted copy color script to external file |
| includes/brand-page-pattern.php | Added PHP opening tag, file header, and heredoc syntax for pattern content |
| build/index.js | Minified build output reflecting text domain changes |
| build/index.asset.php | Updated build version hash |
| assets/frontend.css | Replaced hardcoded styles with CSS custom properties for user customization |
| assets/copy-color.js | New file: extracted copy-to-clipboard functionality from inline script |
| assets/admin.js | New file: admin settings page JavaScript for color picker and styling section toggle |
| README.md | Updated documentation to reflect new styling options and filter-based customization approach |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
ilicfilip
left a comment
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.
Do we want to add Contributors to the readme.txt?
Everything else looks good
Yes we should - add the three of us + the progressplanner user |
|
Added 👍 |
|
Ran the Plugin Review plugin to verify the fixes and got another issue reported, fixed in fc082bb |
No description provided.