-
Notifications
You must be signed in to change notification settings - Fork 129
Added new feature enhancements and fixed orientation issues #56
Conversation
Added changes related to color change of MessageBox, Title, ContentText and addition of Skip button.
private static final int LINE_INDICATOR_COLOR = Color.WHITE; | ||
private static int CIRCLE_INNER_INDICATOR_COLOR = 0xffcccccc; | ||
private static int CIRCLE_INDICATOR_COLOR = Color.WHITE; | ||
private static int LINE_INDICATOR_COLOR = Color.WHITE; |
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.
If these are not static final, you must use camelCase style.
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.
It's better we don't change default values and write separated variables and change them.
.setLineAndPointerColor(Color.WHITE) | ||
.setMessageTitleColor(Color.WHITE) | ||
.setMessageContentTextColor(Color.WHITE) | ||
.setSkip(true,view6) |
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.
Add a space after ,
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.
Why we must set a boolean?
app/build.gradle
Outdated
@@ -4,8 +4,8 @@ android { | |||
compileSdkVersion 26 | |||
defaultConfig { | |||
applicationId "smartdevelop.ir.eram.showcaseview" | |||
minSdkVersion 11 | |||
targetSdkVersion 25 | |||
minSdkVersion 21 |
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.
Why you changed this to 21? we can support lower versions
@@ -111,6 +111,15 @@ public void setColor(int color) { | |||
invalidate(); | |||
} | |||
|
|||
public void setTitleColor(int color) { | |||
if(color!=0) |
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.
Reformat this please
} | ||
|
||
public void setContentTextColor(int color) { | ||
if(color!=0) |
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.
same above
@@ -111,13 +123,27 @@ private GuideView(Context context, View view) { | |||
} | |||
|
|||
mMessageView = new GuideMessageView(getContext()); | |||
skipButton = new TextView(context); | |||
skipButton.setText("SKIP"); |
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.
Remove this hard code and set a variable it doesn't support multi-language and a developer can't change this.
messageViewPadding | ||
); | ||
skipParams = new FrameLayout.LayoutParams(FrameLayout.LayoutParams.WRAP_CONTENT, FrameLayout.LayoutParams.WRAP_CONTENT); | ||
((LayoutParams)skipParams).setMargins(0,0,10,140); |
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.
Remove these magic numbers! why are you using 10 and 140?! they are so weird.
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.
Also you must convert dp to pixel
skipButton.setOnClickListener(new OnClickListener() { | ||
@Override | ||
public void onClick(View view) { | ||
if(setSkip && lastTargetView!=null) |
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.
Reformat
@@ -248,9 +280,23 @@ private boolean isLandscape() { | |||
return display_mode != Configuration.ORIENTATION_PORTRAIT; | |||
} | |||
|
|||
private int checkOrientation(){ | |||
try{ | |||
Display display = ((WindowManager) getContext().getSystemService(Context.WINDOW_SERVICE)).getDefaultDisplay(); |
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.
Reformat
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.
It's more than 100character in one line please use this format or something like that:
Display display = ((WindowManager) getContext().getSystemService(
Context.WINDOW_SERVICE
)).getDefaultDisplay();
break; | ||
|
||
case Surface.ROTATION_270: | ||
((LayoutParams)skipParams).setMargins(0,0,10,0); |
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.
You have a lot of magic numbers
Hi @Adw41t, it's an interesting change but this change has a lot of issues please fix them and notify me. |
break; | ||
|
||
case Surface.ROTATION_90: | ||
((LayoutParams)skipParams).setMargins(10,0,0,0); |
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.
Magic numbers
|
||
switch(checkOrientation()){ | ||
case Surface.ROTATION_0: | ||
((LayoutParams)skipParams).setMargins(0,0,10,140); |
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.
Magic numbers
Hi @mreram, resolved issues as per your suggestions. Thank you for reviewing and guiding!! |
Fixed open issues
#9 : Fixed orientation issue and location in window
#23 : Added Skip for sequence of ShowCaseView
#22 : Added feature to change color of Pointer
#36 : Added feature to change color of Message text, Message Box, and line connecting Message Box and Pointer