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-16512] Android: added padding to TextField #7908

Merged
merged 9 commits into from Jun 27, 2016
Merged

[TIMOB-16512] Android: added padding to TextField #7908

merged 9 commits into from Jun 27, 2016

Conversation

m1ga
Copy link
Contributor

@m1ga m1ga commented Apr 2, 2016

JIRA: https://jira.appcelerator.org/browse/TIMOB-16512

Updating the #5492 PR so can be used with the current branch again. All the credit goes to @bijupmb .

@hansemannn
Copy link
Collaborator

Please add docs, thanks!

@hansemannn
Copy link
Collaborator

Thank you. Please also add the since: "5.4.0" key so people now since when it's available. For the properties that are already available on iOS, you can check this example on how the syntax works.

@hansemannn
Copy link
Collaborator

Will FT now and merge, looks all good! Maybe we should think of moving the padding to a more clean way (on both iOS and Android) with something like this:

var field = Ti.UI.createTextField({
    padding: {
        left: 10,
        right: 10,
        top: 10,
        bottom: 10
    }
});

Does that make sense to you? So we would handle it like we do with listSeparatorInsets and other "bounds-like" API's. But that should be covered in a separate ticket.

@m1ga
Copy link
Contributor Author

m1ga commented Apr 6, 2016

@hansemannn sounds like a good idea and looks better. Should I change it for this PR or leave it like this for the moment so it's the same on iOS?

@hansemannn
Copy link
Collaborator

Let's leave like it is for now and I'll file a ticket for it the next days. Thanks!

@hansemannn
Copy link
Collaborator

You know what, let's do that change in this PR, I will setup one for iOS and we merge both for 5.4.0. Otherwise, we would change and deprecate the whole thing in 5.5.0 / 6.0.0 again and that produces even more work.

@m1ga
Copy link
Contributor Author

m1ga commented Apr 16, 2016

Ok, I'll get the android part ready

@m1ga
Copy link
Contributor Author

m1ga commented Apr 16, 2016

Changed it ;) I've added ios,ipad in the documentation already


- name: bottom
type: Number
summary: Bottom padding
Copy link
Collaborator

Choose a reason for hiding this comment

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

iOS only allows left and right padding because an input should always be vertically centered. We should consider doing the same with Android, so we don't need to handle different behaviors across platforms. Also please deprecate the old properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TextArea will still be TOP/LEFT, TextField is always vertically aligned

@hansemannn
Copy link
Collaborator

Alright! Now you decide: Are you fine with me cherry-picking your commits and setup a new PR including iOS and Android to have the platforms combinded in one PR, or do you want me to merge yours first and setup another one? 😊

@m1ga
Copy link
Contributor Author

m1ga commented Apr 26, 2016

See ti-slack: if its less work with cherry-picking feel free to create a new PR with iOS and Android!

@ashcoding
Copy link
Contributor

Tested and reviewed this with app.js

var win =  Ti.UI.createWindow({
    layout: "vertical",
    backgroundColor: "#fff"
});

var t1 =  Ti.UI.createTextField({
    value: "lllllllllmmmmmmmmmmmmmmlllllllllll",
    padding: {
        left: 20,
        right: 0
    },
    width: 100,
    height: Ti.UI.SIZE,
    borderColor: "#000",
    color: "#000",
    borderWidth: 1,
    top: 10
});


var t2 = Ti.UI.createTextField({
    value: "lllllllllmmmmmmmmmmmmmmlllllllllll",
    padding: {
        right: 20,
        left: 0
    },
    width: 100,
    height: Ti.UI.SIZE,
    borderColor: "#000",
    color: "#000",
    borderWidth: 1,
    top: 10
});

var t3 = Ti.UI.createTextField({
    value: "lllllllllmmmmmmmmmmmmmmlllllllllll",
    padding: {
        left: 20,
        right: 20
    },
    width: 100,
    height: Ti.UI.SIZE,
    borderColor: "#000",
    color: "#000",
    borderWidth: 1,
    top: 10
});

var t4 = Ti.UI.createTextField({
    value: "lllllllllmmmmmmmmmmmmmmlllllllllll",

    width: 100,
    height: 100,
    borderColor: "#000",
    color: "#000",
    borderWidth: 1,
    top: 10
});


win.add(t1);
win.add(t2);
win.add(t3);
win.add(t4);

win.open();

t4.padding = {left: 50, right: 50};

Code working fine and looks good.
@hansemannn I assume that you are preparing iOS PR for parity?
We need to do either:-
a) Cherry-pick this and put it in your iOS PR and update the docs for 6.0.0 ( Cause we are close to freezing for 5.4.0 ) and merge this for 6.0.0
b) Merge this with iOS PR soon.

Is there a link for iOS PR?

@ashcoding
Copy link
Contributor

I think we should push this for 6.0.0. That way, we won't need to rush this. @hansemannn
We can update the docs later.

@ashcoding
Copy link
Contributor

Code reviewed and tested. Approved
Will leave it to @hansemannn to merge when he gets the iOS portion in.

since: "5.4.0"
notes: Use padding instead

- name: paddingRight
Copy link
Collaborator

@hansemannn hansemannn May 5, 2016

Choose a reason for hiding this comment

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

So paddingLeft and paddingRight haven't been documented for Ti.UI.TextArea before? EDIT: It haven't been available for iOS before, so we don't need to deprecate it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So paddingLeft and paddingRight haven't been documented for Ti.UI.TextArea before?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So paddingLeft and paddingRight haven't been documented for Ti.UI.TextArea before?

@ashcoding
Copy link
Contributor

Just a note, this will be for 6.0.0.

int paddingRight = 0;

if (d.containsKey(TiC.PROPERTY_LEFT)) {
paddingLeft = TiConvert.toInt(d, TiC.PROPERTY_LEFT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use TiConvert methods that with a fall back default in case the input is invalid like null and such. Something like this TiConvert.toInt(d, TiC.PROPERTY_LEFT, someDefaultValue).

paddingLeft = tv.getPaddingLeft();
}
if (d.containsKey(TiC.PROPERTY_RIGHT)) {
paddingRight = TiConvert.toInt(d, TiC.PROPERTY_RIGHT);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@ashcoding
Copy link
Contributor

@hieupham007 This looks like it's good to go.

@sgtcoolguy sgtcoolguy modified the milestone: 6.0.0 Jun 17, 2016
@sgtcoolguy sgtcoolguy removed the 6.0.0 label Jun 17, 2016
@ashcoding
Copy link
Contributor

Merging

@build
Copy link
Contributor

build commented Jun 27, 2016

Can one of the admins verify this patch?

@ashcoding ashcoding merged commit 5da41d7 into tidev:master Jun 27, 2016
@m1ga m1ga deleted the android_padding branch June 28, 2016 20:09
@m1ga
Copy link
Contributor Author

m1ga commented Nov 16, 2016

@hansemannn was the iOS part merged into 6.0.0? I've tried to set padding: {left:10,right:10} to a TextField today and it wasn't working. If its not in the documentation needs to be changed

@hansemannn
Copy link
Collaborator

@m1ga Not, yet. Updated in 3f0bb97

@m1ga
Copy link
Contributor Author

m1ga commented Nov 16, 2016

👍

@jpriebe
Copy link

jpriebe commented Jan 17, 2018

It looks like this never got merged. Is it ever going to get merged?

Is there any reason that top/bottom padding isn't included in this PR, the way it is in #5492 ?

BTW -- there's a typo in TextField.yml (3f0bb97) - "descrioption" instead of "description"

@hansemannn
Copy link
Collaborator

@jpriebe This PR was merged on June 26 2016 (see above). For the typo: It is not actually an issue as later commits made the API cross-platform, making the description obsolet. For the top/bottom: Maybe @m1ga knows more about that.

@m1ga
Copy link
Contributor Author

m1ga commented Jan 17, 2018

@jpriebe see this comment: #7908 (comment)
it is because of parity reasons. I've added top/bottom to but removed since it is not part of iOS.

@jpriebe
Copy link

jpriebe commented Jan 17, 2018

Thanks, guys. I was mistakenly looking at TiUIEditText instead of TiUIText.

After some experimentation, I have been able to get padding to work on android, but I have found that vertical padding of 0 doesn't really mean 0 from a visual standpoint. It looks like there is about 12dp of padding on top and bottom that can't be removed. That was leading me to believe that padding didn't work.

When you try to set the size of a text field to something slightly smaller than the default (e.g. 30dp), the padding remains, and the text is cut off.

I guess the bottom line is that there's no way to create a compact TextField on android. (I know, it violates all sorts of usability guidelines to create something that small, but I think there are some legitimate applications.

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

7 participants