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

[TIMOB-24639] Android: Refactor TextInputLayout implementation #9361

Merged
merged 7 commits into from Nov 15, 2017

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Aug 27, 2017

  • TIMOB-25086: Use NestedScrollView to allow nested scrolling (e.g: TableView, ScrollView)
  • TIMOB-24639:
    • Allow animated hintType to be set using attributedHintText, although attributes will be ignored (Android limitation)
    • Always respect height property
    • Set EditText background color to transparent
    • Document the limitations of using hintType: Ti.UI.HINT_TYPE_ANIMATED (e.g: cannot set hintTextColor, again Android limitation)
    • Update documentation
  • TIMOB-24866: Fix padding issue
TEST CASES
var win = Ti.UI.createWindow({title: 'TIMOB-25086', backgroundColor: 'gray'}),
    table = Ti.UI.createTableView(),
    row = Ti.UI.createTableViewRow({
        height: 200
    }),
    textArea = Ti.UI.createTextArea({
        value: 'aaaaaaaabbbbbbbbccccccccddddddddeeeeeeeeffffffff',
        font: {fontSize: '28pt'},
        width: '66%',
        height: Ti.UI.SIZE
    });

row.add(textArea);
table.setData([row]);

win.add(table);
win.open();
var win = Ti.UI.createWindow({title: 'TIMOB-24639', backgroundColor: 'gray', layout: 'vertical'}),
    hintText = 'HINT TEXT',
    textArea_A = Ti.UI.createTextArea({
        value: 'TOP LEFT',
        textAlign: Ti.UI.TEXT_ALIGNMENT_LEFT,
        verticalAlign: Ti.UI.TEXT_VERTICAL_ALIGNMENT_TOP,
        backgroundColor: 'red',
        width: '90%',
        height: '20%',
        top: 5,
        hintText: hintText,
        hintType: Ti.UI.HINT_TYPE_ANIMATED
    }),
    textArea_B = Ti.UI.createTextArea({
        value: 'CENTER (STATIC HINT)',
        textAlign: Ti.UI.TEXT_ALIGNMENT_CENTER,
        verticalAlign: Ti.UI.TEXT_VERTICAL_ALIGNMENT_CENTER,
        backgroundColor: 'red',
        width: '90%',
        height: '20%',
        top: 5,
        hintText: hintText,
        //hintType: Ti.UI.HINT_TYPE_ANIMATED
    }),
    textArea_C = Ti.UI.createTextArea({
        value: 'BOTTOM RIGHT',
        textAlign: Ti.UI.TEXT_ALIGNMENT_RIGHT,
        verticalAlign: Ti.UI.TEXT_VERTICAL_ALIGNMENT_BOTTOM,
        backgroundColor: 'red',
        width: '90%',
        height: '20%',
        top: 5,
        hintText: hintText,
        hintType: Ti.UI.HINT_TYPE_ANIMATED
    });

win.add([textArea_A, textArea_B, textArea_C]);
win.open();

@garymathews garymathews added this to the 7.0.0 milestone Aug 27, 2017
@garymathews garymathews force-pushed the TIMOB-24639_RE branch 2 times, most recently from f0ea621 to f4d7a3f Compare August 28, 2017 14:01
@@ -88,6 +88,8 @@ properties:
The underlying attributed string drawn by the textArea. If set, avoid setting common attributes
in textArea, such as `value`, `color` and `font`, as unexpected behaviors may result.

This has no effect when used with `hintType` Ti.UI.HINT_TYPE_ANIMATED.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this sentence from the "attributedString" property documentation. Hint type does not apply to it.

The TextArea documentation is missing the "attributedHintText" property.

tv.setHint("");
textInputLayout.setHint(hintText);
}
setHintText(type, hintText);
Copy link
Contributor

Choose a reason for hiding this comment

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

Dynamically changing the "hintType" property ignores the "attributedHintText" property. Perhaps it should be written like this?

Object text = d.get(TiC.PROPERTY_ATTRIBUTED_HINT_TEXT);
if (text instanceof AttributedStringProxy) {
	setAttributedStringHint((AttributedStringProxy)text);
} else {
	text = TiConvert.toString(d.get(TiC.PROPERTY_HINT_TEXT), "");
	if (text != null) {
		int type = TiConvert.toInt(newValue);
		setHintText(type, (String)text);
	}
}

nestedScrollView.setFillViewport(true);
nestedScrollView.addView(textInputLayout);

setNativeView(nestedScrollView);
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to double check if Titanium's setTouchEnabled(false) call will gray out the TextField/TextArea like it did before TextInputLayout was implemented. (I forgot to check this before.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I just verified that setting touchEnabled property to false no longer disable TextFields/TextAreas. In fact, they can still be edited.

The best solution to this may be to duplicate what Google has done in their TextInputLayout.java code where they override the setEnabled() method and pass that state to its children via our own NestedScrollView derived class.
https://github.com/android/platform_frameworks_support/blob/master/design/src/android/support/design/widget/TextInputLayout.java#L843

However, there is another issue. Animated hint text (the text above the TextField/TextArea) can be scrolled off the view because we're wrapping the TextInputLayout within a NestedScrollView. The solution may be to dump the NestedScrollView (which fixes the above enabled issue) and have the TiUIEditText class implement the NestedScrollingChild interface.

@@ -135,10 +137,16 @@ public void onLayoutChange(View v, int left, int top, int right, int bottom, int
} else {
tv.setGravity(Gravity.TOP | Gravity.LEFT);
}
tv.setBackgroundColor(Color.TRANSPARENT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Question:
Will this remove the underline even if no custom background was applied?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just confirmed that the underline is no longer shown. We should only hide it if a custom background color/image is applied.

@garymathews garymathews force-pushed the TIMOB-24639_RE branch 3 times, most recently from 42d72fc to 46c6215 Compare August 30, 2017 13:57
@@ -295,22 +296,24 @@ public void propertyChanged(String key, Object oldValue, Object newValue, KrollP
tv.setText(truncateText);
tv.setSelection(cursor);
}
} else if (key.equals(TiC.PROPERTY_BACKGROUND_COLOR)) {
tv.setBackgroundColor(Color.TRANSPARENT);
Copy link
Contributor

Choose a reason for hiding this comment

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

The below needs to be called after changing TextView background color to transparent since we're in an else-if block (the super class' method only gets called in the else block).

super.propertyChanged(key, oldValue, newValue, proxy);

@@ -17,6 +17,7 @@
import ti.modules.titanium.ui.widget.TiUIText;
import android.graphics.drawable.Drawable;
import android.support.design.widget.TextInputLayout;
import android.support.v4.widget.NestedScrollView;
Copy link
Contributor

Choose a reason for hiding this comment

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

This import statement isn't needed anymore, right?

- name: hintType
summary: Hint type to display on the text field.
platforms: [android]
since: {android: "6.2.0"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to version 7.0.0

@jquick-axway
Copy link
Contributor

Side Note:
The TextInputLayout class logs the following warning when giving it an EditText. This can be seen via adb logcat.

TextInputLayout: EditText added is not a TextInputEditText. Please switch to using that class instead.

I resolved it in my nested TextArea scrolling PR ( #9402 ) by modifying TiUIEditText to derive from TextInputEditText class instead of EditText. Feel free to change the derived type on your end if you want to see the results. (I'm okay with resolving the merge conflict on my end.)
https://github.com/appcelerator/titanium_mobile/pull/9402/files#diff-4bf6f4a9c35231bbb4d50c409e2a2453

@garymathews
Copy link
Contributor Author

@jquick-axway Updated PR

@build
Copy link
Contributor

build commented Sep 22, 2017

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

🚫

Tests have failed, see below for more information.

Tests:

Classname Name Time Error
android.Titanium.Network.HTTPClient callbackTestForPOSTMethod 60.044 Error: timeout of 60000ms exceeded
android.Titanium.Network.HTTPClient callbackTestForGETMethod 60.004 Error: timeout of 60000ms exceeded
android.Titanium.XML xmlNodeListChildren 0.001 AssertionError: expected 0 to equal 1
android.Titanium.XML apiXmlNodeProperties 0.004 AssertionError: expected undefined to be a number
android.Titanium.XML xmlCData 0.002 TypeError: Cannot read property 'data' of undefined
android.Titanium.XML xmlNodeCount 0.001 AssertionError: expected true to equal false
ios.Titanium.UI.iOS #constants 0.005 file:///Users/build/Library/Deve
ios.Titanium.UI.View border with only borderColor set 5.003 file:///Users/build/Libra

Generated by 🚫 dangerJS

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

@jquick-axway
Copy link
Contributor

@garymathews, oh wait. The new Ti.UI.HINT_TYPE_ANIMATED constant is currently flagged as deprecated here...
http://docs.appcelerator.com/platform/latest/#!/api/Titanium.UI-property-HINT_TYPE_ANIMATED

@lokeshchdhry
Copy link
Contributor

FR Passed.

  1. TIMOB-24639 - Verified textinputLayout & it works as expected.
  2. TIMOB-24866 - Verified adding padding does not reset its alignment to left/center.

Studio Ver: 4.10.0.201709271713
SDK Ver: 7.0.0 local build
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.3
Appc NPM: 4.2.10
Appc CLI: 6.3.0
Ti CLI Ver: 5.0.14
Alloy Ver: 1.10.7
Node Ver: 7.10.1
Java Ver: 1.8.0_101
Devices: ⇨ google Nexus 5 --- Android 6.0.1
⇨ google Nexus 6P --- Android 8.0.0

@eric34 eric34 merged commit 9391cdc into tidev:master Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants