Feature/421 add color customisations for crop image activity#422
Conversation
Canato
left a comment
Canato
left a comment
There was a problem hiding this comment.
Great work! just some small details ^^
| /** Crop Image background color **/ | ||
| @JvmField | ||
| var activityBackgroundColor: Int = -1 | ||
|
|
||
| /** Toolbar color **/ | ||
| @JvmField | ||
| var toolbarColor: Int = -1 |
There was a problem hiding this comment.
We can add a default value, so we can avoid the logic to check -1
There was a problem hiding this comment.
The thing is if we add a default value it's going to ignore the color taken from styles. Like Primary Color will always be ignored. For toolbar at least. Would that be ok?
There was a problem hiding this comment.
Totally valid point.
Can we get these values here? as default? 🤔
There was a problem hiding this comment.
I just checked this.
We can set default Color WHITE to activity background.
But for toolbar I'm not sure how to fetch the primaryColor from the app themes file.
I found 2 solutions here.
Solution 1
Solution 2
Unfortunately they both require context which is not available in CropImageOptions.
I also tried Resources.getSystem().getColor() method. It has 2 parameters. The first one takes only an id of a color we have defined. So we cannot fetch the colorPrimary attribute using this.
We could also manually set the toolbar color to the primary color stated in the themes file which is currently purpleDark but if in the future this attribute is updated in the themes file it might not be updated for the toolbar
There was a problem hiding this comment.
I will let you decide what makes more sense here. Any solution is good for me
just think about what is easy for our users and for maintenance.
There was a problem hiding this comment.
Is it ok to keep a default value of -1 only for toolbar here? I can't think of any other option that can fetch the primaryColor from the theme file.
So if they don't set the toolbar color it will use the default primary color set in the theme file.
|
One more, we need to add the features here https://github.com/CanHub/Android-Image-Cropper/blob/main/.documentation/features.md |
Alright I'll update that too |
Updated features.md
|
I noticed the background suddenly being white. I had to set it to transparent to get the original theme color back. Why not setting it to transparent by default? |
3 participants
Add the issue linked to this PR
close #421
New Feature
Description:
Added options to customize toolbar color and activity background color of CropImageActivity
Check list for the Code Reviewer:
@Canato I'm a bit rusty with the code. let me know if I need to do anything