-
Notifications
You must be signed in to change notification settings - Fork 215
fix: Add banner image dimension settings reflection on admin vendor page. #3019
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
fix: Add banner image dimension settings reflection on admin vendor page. #3019
Conversation
WalkthroughStreamlines image cropping by removing runtime ratio calculations and exposing a consistent cropControl; makes banner dimensions dynamic via wp.hooks.applyFilters (defaults 625×300) and passes them into media upload handlers and UI hints. Changes
Sequence Diagram(s)mermaid AdminUI->>WP_Media: open media frame with imgSelectOptions + cropControl Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/admin/dashboard/pages/vendor-create-edit/WpMedia.ts(4 hunks)src/admin/dashboard/pages/vendor-create-edit/form/General.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T06:19:41.376Z
Learnt from: mrabbani
Repo: getdokan/dokan PR: 2891
File: src/admin/dashboard/pages/vendors-single/InformationTabs/WithdrawTab.tsx:58-59
Timestamp: 2025-09-19T06:19:41.376Z
Learning: The Dokan codebase uses 'dokan-lite' as the text domain for WordPress internationalization (__() function calls).
Applied to files:
src/admin/dashboard/pages/vendor-create-edit/form/General.tsx
🧬 Code graph analysis (2)
src/admin/dashboard/pages/vendor-create-edit/form/General.tsx (1)
src/admin/dashboard/pages/vendor-create-edit/WpMedia.ts (1)
wpMedia(52-250)
src/admin/dashboard/pages/vendor-create-edit/WpMedia.ts (1)
assets/src/js/product-editor.js (3)
xInit(1293-1300)realWidth(1291-1291)realHeight(1292-1292)
⏰ 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). (4)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
🔇 Additional comments (7)
src/admin/dashboard/pages/vendor-create-edit/form/General.tsx (4)
1-1: LGTM!The
sprintfimport is correctly added to support the dynamic banner dimension label.
319-324: LGTM!The dynamic banner size label correctly uses
sprintfwith the computedbannerWidthandbannerHeightvalues, ensuring the admin-configured dimensions are displayed to users.
334-338: LGTM!The banner change action correctly passes the dynamic
bannerWidthandbannerHeightvalues to thewpMediafunction, ensuring the cropper uses the admin-configured dimensions.
389-395: LGTM!The banner upload action correctly passes the dynamic
bannerWidthandbannerHeightvalues to thewpMediafunction, ensuring consistency with the change action.src/admin/dashboard/pages/vendor-create-edit/WpMedia.ts (3)
58-63: LGTM!Converting
xInitandyInittoconstimproves code immutability and prevents accidental reassignment. The logic correctly uses the providedcroppingWidthandcroppingHeightparameters or falls back to the globalstore_banner_dimensionsettings.
73-74: LGTM!Converting
realWidthandrealHeighttoconstis appropriate since these values are derived from the attachment and should not be modified.
90-104: Verify that always including aspectRatio/maxHeight/maxWidth doesn't break flexible cropping.The change to make
imgSelectOptionsaconstand always includeaspectRatio,maxHeight, andmaxWidthproperties appears to remove previously conditional logic. While this aligns with the General.tsx changes that now pass explicit dimensions, it's important to verify that this doesn't break scenarios where flexible width/height cropping is expected.The AI summary mentions "removed intermediate ratio-based cropping adjustments," but without seeing the old code, it's unclear if this change might affect:
- Cases where
flex_widthorflex_heightis true incropControl(lines 142-149)- The
mustBeCroppedlogic that checks flexibility flags (lines 153-193)Please test the following scenarios to ensure cropping behavior remains correct:
- Upload a banner when admin has set fixed dimensions (flex_width=false, flex_height=false)
- Upload a banner when admin has set flexible width (flex_width=true)
- Upload a banner when admin has set flexible height (flex_height=true)
- Upload a banner when both dimensions are flexible
Verify that the cropper behaves as expected in each case and that the
canSkipCropflag is set appropriately.
|
There’s an existing issue on the vendor dashboard where the size-cropping functionality isn’t working the same way it does on the admin dashboard. Since this PR is intended to fix the cropping issue on the admin side, we’ll proceed with it as is. @MdAsifHossainNadim |
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)
assets/src/js/script.js (1)
351-417: Consider future consistency: profile image still uses legacy crop approach.The banner upload now uses
CustomizeImageCropperwith explicitcropControl, whilecalculateImageSelectOptionsProfilestill relies onapi.HeaderTool.ImageModel.shouldBeCropped(). This is fine for now since profile pictures have fixed 150×150 dimensions, but consider unifying the approach in a future refactor for maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
assets/src/js/script.js(4 hunks)templates/settings/store-form.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
assets/src/js/script.js (1)
assets/src/js/product-editor.js (3)
xInit(1293-1300)realWidth(1291-1291)realHeight(1292-1292)
⏰ 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). (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (1, 3)
🔇 Additional comments (3)
templates/settings/store-form.php (1)
277-283: LGTM! Dynamic banner dimensions with sensible fallbacks.The CSS now correctly reflects admin-configured banner dimensions while ensuring minimum sizes of 625×300 pixels. The
esc_attr()escaping is properly applied.assets/src/js/script.js (2)
177-194: LGTM! Clean refactor to centralized crop control.The cropping decision logic is now properly delegated to
cropControl.mustBeCropped(), and image select options always include aspect ratio constraints for consistent behavior.
337-340: Correct usage ofCustomizeImageCropperwith custom control.The switch to
wp.media.controller.CustomizeImageCropperproperly supports the newcropControlobject, enabling dynamic dimension handling for banner uploads.
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
fix: Add the reflection of admin-configured banner dimensions in admin vendor settings
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes / UI
✏️ Tip: You can customize this high-level summary in your review settings.