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

Added new feature enhancements and fixed orientation issues #56

Closed
wants to merge 5 commits into from

Conversation

Adw41t
Copy link
Contributor

@Adw41t Adw41t commented Aug 14, 2021

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

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;
Copy link
Owner

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.

Copy link
Owner

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)
Copy link
Owner

Choose a reason for hiding this comment

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

Add a space after ,

Copy link
Owner

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
Copy link
Owner

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)
Copy link
Owner

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)
Copy link
Owner

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");
Copy link
Owner

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);
Copy link
Owner

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.

Copy link
Owner

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)
Copy link
Owner

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();
Copy link
Owner

Choose a reason for hiding this comment

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

Reformat

Copy link
Owner

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);
Copy link
Owner

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

@mreram
Copy link
Owner

mreram commented Aug 14, 2021

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);
Copy link
Owner

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);
Copy link
Owner

Choose a reason for hiding this comment

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

Magic numbers

@Adw41t
Copy link
Contributor Author

Adw41t commented Aug 28, 2021

Hi @mreram, resolved issues as per your suggestions. Thank you for reviewing and guiding!!

@Adw41t Adw41t requested a review from mreram September 6, 2021 13:38
@Adw41t
Copy link
Contributor Author

Adw41t commented Oct 15, 2021

Hi @Adw41t, it's an interesting change but this change has a lot of issues please fix them and notify me.

Hi @mreram, fixed issues as per your suggestions. Please review

@Adw41t Adw41t closed this Jan 14, 2022
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.

2 participants