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

[Refactoring] Exude color variables to resources #57

Closed
programmerr47 opened this issue Feb 27, 2018 · 11 comments
Closed

[Refactoring] Exude color variables to resources #57

programmerr47 opened this issue Feb 27, 2018 · 11 comments

Comments

@programmerr47
Copy link

I think it will be better to move color variables into resource file.

  1. It will reduce code duplication

  2. it will increase readability, since I thought through code that warning and error colors are constants, also actualy they not

  3. It will give the user more flexibility to config toast. They will able to rewrite your color resources in their own complex resources files. Suppose, we have aaplication, that for different regions use different color schemes. With code-config-aproach it will request for library user more effort to adjust colors by config changes, that if colors were in resource file

@programmerr47
Copy link
Author

If you don't mind, I can do that

@GrenderG
Copy link
Owner

What do you mean with 2.?

@GrenderG
Copy link
Owner

ping @programmerr47

@programmerr47
Copy link
Author

@GrenderG Hi. Sorry for such delay.

I mean next:
private int DEFAULT_TEXT_COLOR = Toasty.DEFAULT_TEXT_COLOR;
Here is an example of a config field. Since it is written in ALL_CAPS_SNAKE_CASE and it is a common practice for naming constants, you may not notice lack of static final keywords and thus erroneously assume, that it is a constant.
Situation is become more obscure, when you see, that:
private static int DEFAULT_TEXT_COLOR = Color.parseColor("#FFFFFF");
is not just a field of an object, but a field of a class, written in a constant style

See jls for more information. For more easy search page 138, item Field Names :)

@GrenderG
Copy link
Owner

You are right, go ahead with the PR.

@sami286
Copy link
Contributor

sami286 commented Dec 8, 2018

Hi, im working on that.
Should I continue supporting to change default color by Toasty.Config? Or should it be done entirely from resources?

@sami286
Copy link
Contributor

sami286 commented Dec 8, 2018

Just tell me what do you want your library to do and I will implement it :)
@GrenderG

@GrenderG
Copy link
Owner

GrenderG commented Dec 9, 2018

I'm just not sure about how to implement this. I don't want to remove support for @ColorInt, but I know that it would be good to exclude default color variables to resources and / or support theming via color resources, although this can actually be done at this moment using getColor(...) / ContextCompat.getColor(...) methods.

What do you guys think?

@sami286
Copy link
Contributor

sami286 commented Dec 9, 2018

Have a look to the way I implement it, it is now by default getting default color from resources but it is also posible to override them throught the config at run time.

Maybe we shouldn't give an option to override default colors at run time and instead the developers would use Toasty.custom (?)

@GrenderG
Copy link
Owner

Maybe we shouldn't give an option to override default colors at run time and instead the developers would use Toasty.custom (?)

Okay! I agree with this.

@sami286
Copy link
Contributor

sami286 commented Dec 10, 2018

Maybe we shouldn't give an option to override default colors at run time and instead the developers would use Toasty.custom (?)

Okay! I agree with this.

Done! Have a look at it and tell me what you think 😄
#93

@GrenderG GrenderG closed this as completed Jan 3, 2019
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

No branches or pull requests

3 participants