Remove redundant hardcoded sizes on Compose design icons#6478
Open
andremion wants to merge 6 commits into
Open
Remove redundant hardcoded sizes on Compose design icons#6478andremion wants to merge 6 commits into
andremion wants to merge 6 commits into
Conversation
Removes Modifier.size(20.dp) from Icon call sites that paint a 20dp intrinsic stream_design_ic_* drawable. The size is redundant because Icon falls back to the painter's intrinsic size when no size modifier is applied, and removing it lets integrators override the drawable file to drive the rendered size. Touched: ComposerCancelIcon, AudioRecordAttachmentContent PlaybackToggleButton, UnsupportedAttachmentContent, VideoPlaybackControls play/pause toggle, AddMembersScreen back arrow.
Removes Modifier.size(20.dp) from poll-feature icons painted with 20dp intrinsic drawables: PollAnswers add-answer button, PollOptionList remove and error icons, PollSwitchList StepperButton minus/plus icons.
Removes Modifier.size(20.dp) from the share/cancel-share and photos icons in MediaGalleryPreviewScreen, and from the option-row icon in MediaGalleryPreviewOptionsMenu. Every painter consumed by these call sites is a 20dp intrinsic stream_design_ic_* drawable, so the size modifier was redundant.
Removes Modifier.size(20.dp) from both leadingIcon and trailingIcon Icons in ContextualMenuItem. Every caller (message action options, the in-file MenuItem preview) passes a 20dp intrinsic stream_design_ic_* drawable, so the hardcoded size was redundant and blocked integrator drawable overrides from propagating.
The MessageFailedIcon factory paints a 20dp intrinsic stream_design_ic_exclamation_circle_fill, so the .size(20.dp) injected into MessageFailedIconParams.modifier by callers was redundant. Removes it from the two MessageContainer call sites and the PollMessageContent call site.
Contributor
Author
|
@CodeRabbit review |
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (13)
WalkthroughThis PR removes explicit ChangesIcon Sizing Standardization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
SDK Size Comparison 📏
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes AND-1197
Goal
Integrators that swap a
stream_design_ic_*drawable to change an icon's size find the override has no effect: the rendered size is locked by a hardcodedModifier.size(...)on theIcon/Imagethat overrides the painter's intrinsic. This pass removes the size modifier wherever the hardcoded value already matches the drawable's intrinsic, so swapping the drawable actually drives the rendered size.Implementation
All affected painters are 20dp intrinsic
stream_design_ic_*drawables, so the.size(20.dp)was always a no-op visually.Icon/Imagealready fall back to the painter's intrinsic size when no size modifier is set (defaultSizeFor(painter)), so removing the modifier preserves today's rendering and unblocks drawable overrides.Out of scope: every
.size(X.dp)whereXdiffers from the intrinsic (downscales like inline 16dp icons, upscales like 32dp empty-state icons). Those preserve a deliberate visual size and stay as-is.SearchInput'sIconButton.size(20.dp)is also left alone — it sizes the touch target, not the icon, which is a separate concern.Split into five area-scoped commits to keep review focused:
ComposerCancelIcon,AudioRecordAttachmentContentplayback toggle,UnsupportedAttachmentContent,VideoPlaybackControlsplay/pause,AddMembersScreenback arrow.PollAnswersadd-answer button,PollOptionListremove + error icons,PollSwitchListstepper button.MediaGalleryPreviewScreenshare + photos icons,MediaGalleryPreviewOptionsMenuoption-row icon (one wrapper fix covers all four options).ContextualMenuItemwrapper — covers every caller routed throughMessageActionsOptionsItem.MessageFailedIconcallers —MessageContainer(both call sites) andPollMessageContent../gradlew spotlessApply detekt— clean../gradlew apiDumpproduces no.apidiff (only modifier removals).Testing
testCoveragepasses — existing Paparazzi snapshots cover every change exceptVideoPlaybackControlsplay/pause (no test exercises a video preview).Manual verification for
VideoPlaybackControls:Summary by CodeRabbit
Style
Refactor