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

add style, to readme. and also added the properties i just added of h… #31

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

Noitidart
Copy link

…ighlightColor, placeholderColor, color, and disableFullScreenUI

@Noitidart
Copy link
Author

Just added buttonColor option to change color of buttons. And also added cust to style so can change udnerline color, cursor color, and handle color. I also added to the readme on how to use this.

@Noitidart
Copy link
Author

Fixes #25 (prop of disableFullscreenUI is added, and also in both cases (disableFullscreenUI true/false) the keyboard works) and survives orientation changes.

@Noitidart
Copy link
Author

@greedbell this is ready to merge, may you please review.

@@ -198,6 +210,17 @@ public Dialog onCreateDialog(Bundle savedInstanceState) {
return dialog;
}

@Override
public void onStart() {

Choose a reason for hiding this comment

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

why not put code after to line 113?

Copy link
Author

@Noitidart Noitidart Mar 12, 2018

Choose a reason for hiding this comment

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

I wanted to. I actually tried to put it on line 91 - https://github.com/Noitidart/react-native-prompt-android/blob/dd428a864880209114ff9708f76e21e9d1937aa5/android/src/main/java/im/shimo/react/prompt/RNPromptFragment.java#L91 - where we initially work with postive button. However d.getButton returns null until after dialog shows. :(

This person here also encountered same issue - https://tassioauad.com/2016/06/02/dialogfragmentalertdialog-dismiss-automatically-on-click-button/

He says:

You will need to set this in the onResume() method because getButton() will return null until after the dialog has been shown! This should cause your custom action method to only be called once, and the dialog won’t be dismissed by default.

I also tried to ask on Stackoverflow, and other people have the same issue:

https://stackoverflow.com/questions/27520967/how-to-change-the-colour-of-positive-and-negative-button-in-custom-alert-dialog/27521470#comment85378535_27521470

Copy link
Author

Choose a reason for hiding this comment

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

@greedbell @wsong910 would you rather i make the change before accpeting PR? or would you like to find null cases and work around it?

public void onShow(final DialogInterface dialog)
{
input.requestFocus();
((InputMethodManager) alertDialog.getContext().getSystemService(Context.INPUT_METHOD_SERVICE)).showSoftInput(input, 0);

Choose a reason for hiding this comment

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

maybe below code behave better:
InputMethodManager imm = (InputMethodManager) alertDialog.getContext().getSystemService(Context.INPUT_METHOD_SERVICE);
if (imm != null) {
input.requestFocus();
imm.showSoftInput(view, 0);
}

Copy link
Author

Choose a reason for hiding this comment

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

Is there chance that imm is null? If it is, is there a way to wait for it to not be null? Because we always want to show keyboard.

Copy link
Author

Choose a reason for hiding this comment

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

@wsong910 - are you waiting on me to make this change before accepting this PR? Or are you researching when imm can be null so we can force get imm in that situation to ensure keyboard shows?

README.md Outdated
| defaultValue | Default input value | String | |
| placeholder | | String | |
| style | `'default', 'shimo', 'cust'` | String | 'default' |
| disableFullScreenUI | When in landscape mode, don't use fullscreen | Boolean | false |

Choose a reason for hiding this comment

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

Typo, should be disableFullscreenUI

Copy link
Author

Choose a reason for hiding this comment

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

Thanks very much for going through this and catching it. Fixing right now.

public void onStart() {
super.onStart();

if (!mButtonColor.isEmpty()) {

Choose a reason for hiding this comment

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

I just tested this and it crashes as mButtonColor is null when not specifying this prop or when passed an empty string.

In JS buttonColor: options.buttonColor || null will set buttonColor to null if not specified or if it's an empty string.

Copy link
Author

Choose a reason for hiding this comment

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

Oh shoot i forgot to test for null. Thanks so much on this right away.

Copy link
Author

Choose a reason for hiding this comment

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

fixed thanks @jeanregisser!

@Noitidart
Copy link
Author

Added android-with-options screenshot to readme.

README.md Outdated
defaultValue | Default input value | String | ''
placeholder | | String | ''
##### colorString
Is one of the following: `'red', 'blue', 'green', 'black', 'white', 'gray', 'cyan', 'magenta', 'yellow', 'lightgray', 'darkgray', 'grey', 'lightgrey', 'darkgrey', 'aqua', 'fuchsia', 'lime', 'maroon', 'navy', 'olive', 'purple', 'silver', and 'teal'`
Copy link

@jeanregisser jeanregisser Mar 13, 2018

Choose a reason for hiding this comment

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

You could actually support the full color format (named, rgb, rgba, hsl, etc) available in React Native by processing the color passed to the components like this:

import processColor from 'react-native/Libraries/StyleSheet/processColor';

highlightColor: processColor(options.highlightColor),
placeholderColor: processColor(options.placeholderColor),
color: processColor(options.color),
buttonColor: processColor(options.buttonColor)

Then just don't call Color.parseColor in Java but just take the Int value.

Copy link
Author

Choose a reason for hiding this comment

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

Oh sweet! Is it possible to process color on native side? Like RCTConvert in ios?

Choose a reason for hiding this comment

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

No, RCTConvert on iOS just transforms the int to a UIColor, so it can be conveniently used.
But Android native views can directly deal with Int.

Copy link
Author

Choose a reason for hiding this comment

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

Update to support full color format! Super awesome!! would be awesome to learn how to do this on native side too.


if (mButtonColor != null && !mButtonColor.isEmpty()) {
AlertDialog d = (AlertDialog) getDialog();
d.getButton(AlertDialog.BUTTON_POSITIVE).setTextColor(Color.parseColor(mButtonColor));

Choose a reason for hiding this comment

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

Since you now use processColor on the JS side, you need to remove all Color.parseColor and just take the Int value from the arguments.

Copy link
Author

Choose a reason for hiding this comment

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

done

@jeanregisser
Copy link

Tried your latest changes @Noitidart this works great! 👍

@Noitidart
Copy link
Author

Super cool thanks @jeanregisser! :) I also learned a bunch thanks to you!

@Noitidart
Copy link
Author

Bum, is there anything I need to do to get this accepted?

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.

3 participants