[ALOY-1703] Add several XML attribute aliases #953
Conversation
There was a problem hiding this comment.
I think the verticalAlign is technically a change of behaviour, so I'm not too keen on that.
What's the desire for adding the src alias, is it purely because it's less characters than image? I'm not really a fan of adding an undocumented alias purely to avoid typing 2 characters.
| } | ||
|
|
||
| if ( node.getAttribute('verticalAlign') === 'center' ) { | ||
| node.setAttribute('verticalAlign', 'Titanium.UI.TEXT_VERTICAL_ALIGNMENT_CENTER'); |
There was a problem hiding this comment.
Based on Josh's comment, this is a change in behaviour correct?
There was a problem hiding this comment.
The thing is, I am not talking about changing anything with the SDK. This is simply an intuitive way to be able to center text and it mirrors what you can do with textAlign. I think NOT doing this is actually more harmful as most developers are going to assume that "center" can be used and won't find out until their build breaks.
There was a problem hiding this comment.
But there is a behaviour change here, verticalAlign: 'center' currently does one thing (which I agree doesn't do what I would expect) and here we're changing it do something completely different.
Alloy has no special handling for textAlign, I think it "just works" due to TEXT_ALIGNMENT_CENTER having a value of center, whereas TEXT_VERTICAL_ALIGNMENT_CENTER has a value (on Android) of middle so it doesn't work.
If we're adding special handling for center, why not add the other variants?
There was a problem hiding this comment.
The thing is, no one knows about "middle" unless you dig into the source code. "center" makes sense because it is part of the name TEXT_VERTICAL_ALIGNMENT_CENTER and the fact that TEXT_ALIGNMENT_CENTER can be aliased by using "center". If we want to change the value to TEXT_VERTICAL_ALIGNMENT_MIDDLE, then "middle" would make sense.
There was a problem hiding this comment.
I wasn't saying people should know about middle, it was just an explanation of why this weird behaviour happens.
I tried testing this change, and it looks like it only works when used in views and not in styles?
| @@ -44,6 +44,10 @@ function parse(node, state, args) { | |||
| } | |||
| } | |||
|
|
|||
| if ( node.getAttribute('verticalAlign') === 'center' ) { | |||
| node.setAttribute('verticalAlign', 'Titanium.UI.TEXT_VERTICAL_ALIGNMENT_CENTER'); | |||
There was a problem hiding this comment.
Similar change in behaviour comment
There was a problem hiding this comment.
What's the desire for adding the
srcalias, is it purely because it's less characters thanimage? I'm not really a fan of adding an undocumented alias purely to avoid typing 2 characters.
I think src or source makes much more sense. That would align us more with HTML, NativeScript, ReactNative, Xamarin, etc
There was a problem hiding this comment.
If we want to use src that change should come in the SDK imo
| @@ -86,6 +86,10 @@ function parse(node, state, args) { | |||
| postCode += '<%= parentSymbol %>.add(' + parentSymbol + ');'; | |||
| } | |||
|
|
|||
| if ( node.getAttribute('verticalAlign') === 'center' ) { | |||
| node.setAttribute('verticalAlign', 'Titanium.UI.TEXT_VERTICAL_ALIGNMENT_CENTER'); | |||
There was a problem hiding this comment.
Similar change in behaviour comment
| @@ -136,6 +136,10 @@ function parse(node, state, args) { | |||
| } | |||
| } | |||
|
|
|||
| if ( node.getAttribute('verticalAlign') === 'center' ) { | |||
| node.setAttribute('verticalAlign', 'Titanium.UI.TEXT_VERTICAL_ALIGNMENT_CENTER'); | |||
There was a problem hiding this comment.
Similar change in behaviour comment
|
@brentonhouse I like the idea of making this simpler. But I think as it stands, IMO the potential confusion from |
|
To me it would be better to integrate these aliases into the SDK instead of Alloy. It is hard to explain in documentation some things exist in alloy only. I also think introducing things like a src property on an ImageView will cause more confusion than it solves. There already is confusion with html, we shouldn't want to confirm that. |
|
understood. |
No description provided.