Skip to content
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

Pr/cropper text label support #382

Merged
merged 5 commits into from
Jun 10, 2022

Conversation

bhagyae5308
Copy link
Contributor

The changes are related to #381

  • Added a helper text on top of crop overlay which moves along with it
  • Exposed xml attrs and setter to update the copy text styles

Adding sample working video after the change

Cropper_sticky_label.mp4

- Added a helper text on top of crop overlay which moves along with it
- Exposed xml attrs and setter to update the copy text styles
@bhagyae5308 bhagyae5308 requested a review from a team as a code owner June 10, 2022 04:45
* Text size for text label over crop overlay UI
* default: 20sp
*/
private var mCropLabelTextSize = 20f

Check warning

Code scanning / detekt

Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers.

This expression contains a magic number. Consider defining it to a well named constant.
/** Text to show over text label over crop overlay */
private var cropLabelText: String = ""
/** Text color to apply over text label over crop overlay */
private var cropLabelTextSize: Float = 20f

Check warning

Code scanning / detekt

Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers.

This expression contains a magic number. Consider defining it to a well named constant.
@@ -373,6 +391,9 @@
showIntentChooser = false
intentChooserTitle = null
intentChooserPriorityList = listOf()
cropperLabelTextSize = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_SP, 20f, dm)

Check warning

Code scanning / detekt

Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers.

This expression contains a magic number. Consider defining it to a well named constant.
Copy link
Member

@Canato Canato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work!

Only comments are related to stuff you could not know before anyway, some old Java way of doing we are trying to improve.

Please update the CHANGELOG

Comment on lines 37 to 48
/**
* Creates the paint object for drawing text label over crop overlay
*/
private fun getTextPaint(options: CropImageOptions): Paint {
return Paint().apply {
strokeWidth = 1f
textSize = options.cropperLabelTextSize
style = Paint.Style.FILL
textAlign = Paint.Align.CENTER
this.color = options.cropperLabelTextColor
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change to:
/** Creates the paint object for drawing text label over crop overlay */

private fun getTextPaint(options: CropImageOptions): Paint = , removing return

@@ -92,6 +104,8 @@ class CropOverlayView
/** The Paint used to darken the surrounding areas outside the crop area. */
private var mBackgroundPaint: Paint? = null

private var mTextLabelPaint: Paint? = null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this m is the old java way. And we didn't remove every where, please use textLabelPaint

- Old java style coding updated as per feedback
if (isCropLabelEnabled) {
val rect = mCropWindowHandler.getRect()
var xCoordinate = (rect.left + rect.right) / 2
var yCoordinate = rect.top - 50

Check warning

Code scanning / detekt

Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers.

This expression contains a magic number. Consider defining it to a well named constant.
Added missing activity declaration over Manifest.xml
@@ -20,7 +20,7 @@
<activity android:name="com.canhub.cropper.sample.SampleResultScreen" />
<activity android:name="com.canhub.cropper.CropImageActivity" />
<activity
android:name="com.canhub.cropper.sample.extend_activity.app.SExtendActivity"
android:name="com.canhub.cropper.sample.SampleCustomActivity"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Canato It was an existing crash, so fixed it along with other changes

@bhagyae5308 bhagyae5308 requested a review from Canato June 10, 2022 11:56
Copy link
Member

@Canato Canato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!

@Canato Canato merged commit 19aad12 into CanHub:main Jun 10, 2022
@Canato Canato linked an issue Jun 10, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] - Helper text over crop overlay which alongs with it
2 participants