Fix: vendor dashboard store banner cropping error #3039
Conversation
📝 WalkthroughWalkthroughReplaced the CustomizeImageCropper with a new Cropper controller in the media frame, reworked cropping eligibility to use shouldBeCropped (treating flexible width/height as non-cropping), expanded imgSelectOptions with sizing/coordinate fields, and updated default width fallback to dokan.store_banner_dimension. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
juah255
added
Needs: Dev Review
![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
🧹 Nitpick comments (1)
assets/src/js/script.js (1)
177-187: Simplify the redundant condition in the cropping logic.The second conditional at line 183 checks
!shouldBeCropped, but this is redundant when the first condition (flexWidth && flexHeight) setsshouldBeCroppedto false. Consider whether the condition should useelse ifto clarify that these are mutually exclusive paths, or restructure to remove the logical redundancy.Since the second condition already assigns the same value in both cases, simplifying the logic would improve clarity without changing behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assets/src/js/script.js
🧰 Additional context used
🧬 Code graph analysis (1)
assets/src/js/script.js (1)
assets/src/js/product-editor.js (3)
realHeight(1292-1292)realWidth(1291-1291)xInit(1293-1300)
⏰ 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: api tests (1, 1)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (1, 3)
🔇 Additional comments (3)
assets/src/js/script.js (3)
170-173: LGTM: Variable declarations reorganized.The variables are now declared upfront and assigned values later, which improves code clarity.
189-203: LGTM: Cropper options properly expanded.The additional properties (
instance,persistent,imageWidth,imageHeight,aspectRatio,maxHeight,maxWidth) are standard options for the WordPress image cropper and provide better control over the cropping interface.
346-346: Verifywp.media.controller.Cropperis available in the minimum WordPress version supported.The
wp.media.controller.Cropperclass has been available since WordPress 4.2.0, while thewp.media.controller.CustomizeImageCropperis specifically for the WordPress Customizer context (available since 4.3.0). Before merging, confirm:
- That the plugin's minimum WordPress version supports 4.2.0 or higher
- That this resolves the cropping error mentioned in the PR
assets/src/js/script.js
Outdated
| new wp.media.controller.Cropper({ | ||
| imgSelectOptions: settings.calculateImageSelectOptions, | ||
| control: cropControl | ||
| suggestedWidth: dokan?.store_banner_dimension?.width || 5000 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for store_banner_dimension and related context
echo "=== Searching for store_banner_dimension usage ==="
rg -n "store_banner_dimension" --type=js --type=php -B 3 -A 3
echo ""
echo "=== Searching for file around line 348 in script.js ==="
fd -e js -n "script.js" -x wc -l {}
echo ""
echo "=== Checking if the file exists and getting context around line 348 ==="
if [ -f "assets/src/js/script.js" ]; then
sed -n '340,360p' "assets/src/js/script.js"
fiRepository: getdokan/dokan
Length of output: 10279
Inconsistent fallback value for suggestedWidth in media uploader.
The fallback value of 5000 on line 348 is inconsistent with the rest of the codebase. Line 343–344 immediately above uses dokan.store_banner_dimension.width without a fallback, and other parts of the codebase (Vue components, backend) consistently use 625 as the width default. Dokan's documented default store banner width is 625.
If dokan.store_banner_dimension.width can be undefined, the fallback should match the established default of 625 for consistency. If it's guaranteed to be defined from the backend, the optional chaining and fallback are unnecessary. Clarify which case applies and adjust accordingly.
🤖 Prompt for AI Agents
In assets/src/js/script.js around line 348, the media uploader uses a fallback
width of 5000 for suggestedWidth which conflicts with the rest of the codebase
and Dokan's documented default of 625; update this to use the consistent default
by replacing the 5000 fallback with 625, or remove the optional chaining +
fallback entirely if dokan.store_banner_dimension.width is guaranteed by the
backend (choose one approach and apply it consistently so suggestedWidth becomes
either dokan.store_banner_dimension.width || 625 or simply
dokan.store_banner_dimension.width).
|
@juah255 , Thanks for your PR. Could you pls try to reproduce this issue in Old WP version e.i. 6.8 or older? |
assets/src/js/script.js
Outdated
| new wp.media.controller.Cropper({ | ||
| imgSelectOptions: settings.calculateImageSelectOptions, | ||
| control: cropControl | ||
| suggestedWidth: dokan?.store_banner_dimension?.width || 5000 |
There was a problem hiding this comment.
Pls replace the 5000 with the default value of Dokan Settings.
mrabbani
added
Needs: Author Reply
and removed
Needs: Dev Review
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
assets/src/js/script.js (1)
348-348: Fallback value should be625to match Dokan's documented default.The fallback was changed from
5000to500, but as previously noted, Dokan's documented default store banner width is625. Additionally, lines 343-344 accessdokan.store_banner_dimensionwithout optional chaining or fallbacks, creating an inconsistency.🔎 Suggested fix
- suggestedWidth: dokan?.store_banner_dimension?.width || 500 + suggestedWidth: dokan?.store_banner_dimension?.width || 625Or, for consistency with lines 343-344, if
dokan.store_banner_dimension.widthis guaranteed to be defined:- suggestedWidth: dokan?.store_banner_dimension?.width || 500 + suggestedWidth: dokan.store_banner_dimension.width
🧹 Nitpick comments (1)
assets/src/js/script.js (1)
177-187: Consider simplifying the cropping eligibility logic.The current two-step approach works correctly but has some redundancy. The
!shouldBeCroppedcheck on line 183 is always true when the firstifblock was executed.🔎 Suggested simplification
- var shouldBeCropped = true; - - if (flexWidth && flexHeight) { - shouldBeCropped = false; - } - - if (!shouldBeCropped || (realWidth === xInit && realHeight === yInit)) { - shouldBeCropped = false; - } - - controller.set('canSkipCrop', !shouldBeCropped); + var shouldBeCropped = !(flexWidth && flexHeight) && + !(realWidth === xInit && realHeight === yInit); + + controller.set('canSkipCrop', !shouldBeCropped);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assets/src/js/script.js
🧰 Additional context used
🧬 Code graph analysis (1)
assets/src/js/script.js (1)
assets/src/js/product-editor.js (3)
realHeight(1292-1292)realWidth(1291-1291)xInit(1293-1300)
🔇 Additional comments (2)
assets/src/js/script.js (2)
189-203: LGTM!The expanded
imgSelectOptionsobject properly configures the cropper with appropriate dimension constraints and coordinate boundaries for the store banner.
346-350: Good fix: Switching to the standard Cropper controller.Using
wp.media.controller.Cropperinstead ofCustomizeImageCropperis the correct approach for the vendor dashboard context, asCustomizeImageCropperis designed specifically for the WordPress Customizer and may have dependencies or behaviors that cause errors outside that context.However,
suggestedWidthshould not be passed directly to the Cropper controller. According to WordPress media API design,suggestedWidthandsuggestedHeightare UI hints meant for theLibrarycontroller state—they help the media frame decide whether cropping is necessary. The Cropper controller acceptsimgSelectOptions(confirmed), butsuggestedWidthpassed to the Cropper itself will likely be ignored or cause unexpected behavior. RemovesuggestedWidthfrom the Cropper initialization on line 349.
juah255
added
Needs: Dev Review
All Submissions:
Changes proposed in this Pull Request:
This PR resolves an issue where vendor dashboard store banner cropping was causing an error
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Fix: vendor dashboard store banner cropping error
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
✏️ Tip: You can customize this high-level summary in your review settings.