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

[ALOY-1547] Added support for setting font from Alloy xml view #764

Closed
wants to merge 8 commits into from

Conversation

brentonhouse
Copy link
Contributor

@brentonhouse brentonhouse commented Feb 29, 2016

if (args.createArgs && (args.createArgs.fontSize || args.createArgs.fontStyle || args.createArgs.fontFamily || args.createArgs.fontWeight || args.createArgs.textStyle)) {
args.createArgs.font = args.createArgs.font || {};
_.defaults(args.createArgs.font, {
fontSize: args.createArgs.fontSize || args.createArgs.font.fontSize
Copy link
Contributor

Choose a reason for hiding this comment

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

I think font.fontSize should come first
Also, where are the others here?

@FokkeZB
Copy link
Contributor

FokkeZB commented Mar 1, 2016

I like the idea but have two notes:

  • This doesn't support best practice to set styles in TSS not in XML
  • It would be better to have a generic way to set object properties via XML, e.g.:
<Label font:fontSize=10 />

@FokkeZB
Copy link
Contributor

FokkeZB commented Mar 1, 2016

@brentonhouse could you create a JIRA ticket so we can discuss this beyond code?

@brentonhouse
Copy link
Contributor Author

Sorry, I should have linked to the JIRA ticket. Here it is: https://jira.appcelerator.org/browse/AC-696

@brentonhouse
Copy link
Contributor Author

I was thinking that font.fontSize might come from the style (which would be overwritten) but they will come in the separate style objects. I can change it and add it to the PR.

@brentonhouse
Copy link
Contributor Author

I like the idea of using xml prefixes to distinguish things. I think that would have been a good way to implement module and namespace support in Alloy tags. Are there a lot of other child object, other than font, that need to be set in xml? I noticed that setting font seems to be one of the few commonly used properties that can't be set currently using the normal xml attributes.

@FokkeZB
Copy link
Contributor

FokkeZB commented Mar 1, 2016

-deleted-

Let's continue discussion on https://jira.appcelerator.org/browse/AC-696

@brentonhouse
Copy link
Contributor Author

@feons @hansemannn

Just curious if this has any life left in it... I've gotta say this is still probably one of my number one things I have to workaround with Titanium. A lot of times I am trying to quickly create views and I just want to be able to change the fontFamily or fontSize in the xml instead of having to create a .tss file or a JavaScript controller. This is especially helpful when you are trying to quickly prototype something and neither the .js file nor the .tss file exist.

I work around this by always using custom label's or custom textField's but I would absolutely LOVE it if this was merged into alloy so I could do this out of the box. I think this would also significantly help with lowering some of the barrier to entry by removing some of the complexity that has always existed around font's in Alloy.

@hansemannn hansemannn requested a review from feons February 9, 2018 16:07
@hansemannn hansemannn changed the title Added support for setting font from Alloy xml view [ALOY-1547] Added support for setting font from Alloy xml view Feb 9, 2018
@brentonhouse
Copy link
Contributor Author

@feons @ewanharris - is there anything that would keep us from merging this before the next Alloy release?

@hansemannn
Copy link
Contributor

Lets review, document and merge this please. We would use it at Lambus as well, really good feature for clean XML definitions

@ewanharris
Copy link
Contributor

@brentonhouse, I'd like to find some time take a read through the related tickets/discussions before moving on this. I don't see this making into the next Alloy release in CLI 7.0.10

@hansemannn
Copy link
Contributor

@ewanharris Any update here?

Copy link
Contributor

@ewanharris ewanharris left a comment

Choose a reason for hiding this comment

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

Given that #765 exists my preference here is to take that solution over this one. I'm not that comfortable with adding special handling of the font settings and the precedence this sets for allowing similar properties in future that might cause us headaches further down the line if a potential naming collision occurs.

@ewanharris
Copy link
Contributor

@brentonhouse, @hansemannn do you have any objections to the above? Would you be fine with considering this solved by #765?

@hansemannn
Copy link
Contributor

I personally strongly prefer font:fontSize over font.fontSize, simply because we already have this in other cases like item-templates (title:text) and the dot-notation looks like invalid XML.

@brentonhouse
Copy link
Contributor Author

My preference is to have both (i.e. fontFamily and font.Family) but I know I am in the minority on that one.

On the . vs : for #765 -- I prefer the .
After thinking about this over the last few years, the : is reserved for xml namespaces and it makes more sense for that to be used for platform #922 rather than arbitrary objects. The . is valid for xml attributes and then can be used for setting any child objects as necessary.

@ewanharris
Copy link
Contributor

I agree with Brenton in that I prefer dot notation here, I do agree it's not as XML-y but it does align more with what's being done (i.e. setting a key of an object). I guess it could be argued that the usage in an ItemTemplate is maybe more in line with the platform namespacing added in #922

@ewanharris
Copy link
Contributor

Going to close this one out in favour of #765

@ewanharris ewanharris closed this Mar 28, 2019
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

5 participants