-
Notifications
You must be signed in to change notification settings - Fork 31
feat: Image plugin refactored for simpler size control #316
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
feat: Image plugin refactored for simpler size control #316
Conversation
…plugin, and purge ocurrences
…n, and purge ocurrences
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR reorganizes the image plugin UI by consolidating size-related controls into a new “Dimensions” tab, removes unused autoscaling and original-image options, updates help text for clarity, and cleans up related model and migration logic. Entity relationship diagram for image plugin form fields after PRerDiagram
IMAGE_PLUGIN_FORM {
INTEGER width
INTEGER height
STRING alignment
STRING link_attributes
BOOLEAN use_crop
BOOLEAN use_upscale
STRING use_responsive_image
STRING thumbnail_options
}
IMAGE_PLUGIN_FORM ||--o| THUMBNAIL_OPTIONS : uses
IMAGE_PLUGIN_FORM ||--o| RESPONSIVE_IMAGE : uses
Class diagram for updated ImagePlugin form fieldsclassDiagram
class ImagePluginForm {
- width: IntegerField
- height: IntegerField
- alignment: ChoiceField
- link_attributes: CharField
- use_crop: BooleanField
- use_upscale: BooleanField
- use_responsive_image: ChoiceField
- thumbnail_options: ChoiceField
+ clean()
}
%% Removed fields
%% use_automatic_scaling, use_no_cropping
%% Updated help text for width, height, use_crop, use_upscale
Class diagram for updated Image model logicclassDiagram
class Image {
+ get_size(width, height)
+ img_src()
- use_automatic_scaling (removed)
- use_no_cropping (removed)
}
%% get_size and img_src no longer depend on use_automatic_scaling or use_no_cropping
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Resolve the leftover “ask” comments and ensure the removal of use_automatic_scaling/use_no_cropping is consistently handled in image_save and bootstrap4_migration.
- Fix the tab title overflow by either using a shorter label or adjusting the tab container’s width/CSS to prevent wrapping.
- Confirm that removing the legacy scaling and cropping fields won’t break existing plugin instances—consider a data migration or default fallback for backward compatibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Resolve the leftover “ask” comments and ensure the removal of use_automatic_scaling/use_no_cropping is consistently handled in image_save and bootstrap4_migration.
- Fix the tab title overflow by either using a shorter label or adjusting the tab container’s width/CSS to prevent wrapping.
- Confirm that removing the legacy scaling and cropping fields won’t break existing plugin instances—consider a data migration or default fallback for backward compatibility.
## Individual Comments
### Comment 1
<location> `djangocms_frontend/contrib/image/forms.py:152` </location>
<code_context>
- )
# upscale and crop work together
# throws validation error if other cropping options are selected
use_crop = forms.BooleanField(
label=_("Crop image"),
required=False,
- help_text=_("Crops the image according to the thumbnail settings provided in the template."),
+ help_text=_("Crops the image rather than resizing"),
)
use_upscale = forms.BooleanField(
</code_context>
<issue_to_address>
**suggestion:** The help text for 'use_crop' is now less descriptive.
The previous help text clarified that cropping is based on template thumbnail settings, which may be important for user understanding. Please review if the new wording is sufficiently informative.
```suggestion
help_text=_("Crops the image according to the thumbnail settings provided in the template."),
```
</issue_to_address>
### Comment 2
<location> `djangocms_frontend/contrib/image/image_save.py:32` </location>
<code_context>
img.config.update(
{
"picture": {"pk": image_obj.pk, "model": "filer.image"},
- "use_no_cropping": True,
+ "use_no_cropping": True, # ask
}
)
</code_context>
<issue_to_address>
**question:** The comment '# ask' suggests uncertainty about the use of 'use_no_cropping'.
If 'use_no_cropping' is deprecated or its behavior has changed, please document its current usage to prevent confusion.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #316 +/- ##
==========================================
- Coverage 89.13% 89.00% -0.13%
==========================================
Files 124 124
Lines 3386 3383 -3
Branches 288 287 -1
==========================================
- Hits 3018 3011 -7
- Misses 252 256 +4
Partials 116 116 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@marbru Once you remove the |
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.
Well done, @marbru
When triaging this issue we realized that the UI for customizing image dimensions is confusing, and it's applying unexpected defaults silently (eg, overriding user height to keep aspect ratio).
This PR does not fix the bug, but makes the UI a bit clearer:
Before, the UI looked like this:
And now it looks like this:
Issues / notes
Summary by Sourcery
Improve the image plugin UI by consolidating dimension controls into a clearer Dimensions tab, removing unused legacy options, and updating help text for better user guidance
Enhancements:
Chores: