Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Justin Mclean]

Hi,

> Well, then maybe you can help us figure out why.  Upthread there are
> suggestions to check the manifest, etc.  What do you see in yours?

I sorted out the issue in the end. The alternate AllXXX class did suit our 
needs in terms of size / performance / quality so we went with an alternative 
method of using CSS and classList.add and classList.remove calls.

I’ve reverted the change.

Thanks,
Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Alex Harui]

Well, then maybe you can help us figure out why.  Upthread there are
suggestions to check the manifest, etc.  What do you see in yours?

-Alex

On 7/6/17, 4:27 PM, "Justin Mclean"  wrote:

>Hi,
>
>> I just tried swapping SimpleCSSValuesImpl for AllCSSValuesImpl in the
>> develop branch in examples/flexjs/DataBindingExample and it worked for
>>me.
>> 
>> Does it still not work for you?
>
>It still does not work for me.
>
>Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Justin Mclean]

Hi,

> I just tried swapping SimpleCSSValuesImpl for AllCSSValuesImpl in the
> develop branch in examples/flexjs/DataBindingExample and it worked for me.
> 
> Does it still not work for you?

It still does not work for me.

Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Alex Harui]

I just tried swapping SimpleCSSValuesImpl for AllCSSValuesImpl in the
develop branch in examples/flexjs/DataBindingExample and it worked for me.

Does it still not work for you?

-Alex

On 6/29/17, 3:40 AM, "Harbs"  wrote:

>Check your basic-manifest.xml for Core.
>
>Do you see class="org.apache.flex.core.AllCSSValuesImpl”/>?
>
>> On Jun 29, 2017, at 12:54 PM, Justin Mclean 
>>wrote:
>> 
>> Hi,
>> 
>> The class is there I’m on the latest version of develop of all 3 repos
>>and I’m still getting that compile error. Did you actually try using
>>that class / compiling it?
>> 
>> Thanks,
>> Justin
>

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Harbs]

Yes.

> On Jun 29, 2017, at 12:54 PM, Justin Mclean  wrote:
> 
> Did you actually try using that class / compiling it?

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Harbs]

Check your basic-manifest.xml for Core.

Do you see 

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Justin Mclean]

Hi,

The class is there I’m on the latest version of develop of all 3 repos and I’m 
still getting that compile error. Did you actually try using that class / 
compiling it?

Thanks,
Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Harbs]

No. It’s there.

Make sure you have commit af1f5c9 in your build of Core.

> On Jun 29, 2017, at 12:21 PM, Justin Mclean  wrote:
> 
> Hi,
> 
>> That’s why you should use 
> 
> You can’t in this case as it doesn’t compile with the following error:
> 
> This tag could not be resolved to an ActionScript class. It will be ignored.
> 
>
> 
> So somehow you need to include one on one platform and one on the other in 
> MXML I’m not sure if our compiler currently supports that.
> 
> Thanks,
> Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Justin Mclean]

Hi,

> That’s why you should use 

You can’t in this case as it doesn’t compile with the following error:

This tag could not be resolved to an ActionScript class. It will be ignored.


 
So somehow you need to include one on one platform and one on the other in MXML 
I’m not sure if our compiler currently supports that.
  
Thanks,
Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Harbs]

I’m not sure what your point is.

That’s why you should use 

SimpleCSSValuesImpl is supposed to be a simple implementation which works the 
same in Flash and HTML. AllCSSValuesImpl is supposed to be an implementation 
which works with valid HTML CSS.

Thanks,
Harbs

> On Jun 29, 2017, at 11:34 AM, Justin Mclean  wrote:
> 
> Hi,
> 
>> A font weight of 600 will fail in Flash.
> 
> It compiles with no warnings and displays the text correctly. There is no run 
> time error but if you mean by “fail” it doesn't set the text to font weight 
> to 600 then that’s correct. That is expected as 600 is not a supported value 
> for font weight in AS / flash.
> 
> However without the fix we have the same issues on JS in that it will ignore 
> the font weight being set to 600. This is unexpended behaviour in that it’s 
> ignoring a valid value. Again it will compile and give no warnings or run 
> time errors.
> 
> Here’s the code for a full application if you want to try it out.
> 
> 
> http://ns.adobe.com/mxml/2009;
>xmlns:js="library://ns.apache.org/flexjs/basic">
> 
>
>
>
> 
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> 
> 
> 
> With my fix it works as expected in JS i.e. the two lines are different sizes.
> 
> Thanks,
> Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Justin Mclean]

Hi,

> A font weight of 600 will fail in Flash.

It compiles with no warnings and displays the text correctly. There is no run 
time error but if you mean by “fail” it doesn't set the text to font weight to 
600 then that’s correct. That is expected as 600 is not a supported value for 
font weight in AS / flash.

However without the fix we have the same issues on JS in that it will ignore 
the font weight being set to 600. This is unexpended behaviour in that it’s 
ignoring a valid value. Again it will compile and give no warnings or run time 
errors.

Here’s the code for a full application if you want to try it out.


http://ns.adobe.com/mxml/2009;
xmlns:js="library://ns.apache.org/flexjs/basic">
























With my fix it works as expected in JS i.e. the two lines are different sizes.

Thanks,
Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Harbs]

A font weight of 600 will fail in Flash. It’s CSS that only works in HTML.

> On Jun 29, 2017, at 10:49 AM, Justin Mclean  wrote:
> 
> Hi,
> 
>> Considering we have AllCSSValuesImpl, I think that should be the default for 
>> apps which are expecting HTML CSS.
> 
> It’s not about HTTML CSS.
> 
> This code:
> 
> 
> 
> 
> 
> 
> 
> 
> 
> Will compile with no warnings but fail to work without the fix.
> 
> Thanks,
> Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Justin Mclean]

Hi,

> Considering we have AllCSSValuesImpl, I think that should be the default for 
> apps which are expecting HTML CSS.

It’s not about HTTML CSS.

This code:


 
 
 
 
 


Will compile with no warnings but fail to work without the fix.

Thanks,
Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Harbs]

That was before his response. Sorry I never responded to Alex, but once he 
explained himself, I do agree with what he wrote.

Considering we have AllCSSValuesImpl, I think that should be the default for 
apps which are expecting HTML CSS.

Just swap that out in your app, and everything should be fine.

Harbs

> On Jun 29, 2017, at 10:26 AM, Justin Mclean  wrote:
> 
> Hi,
> 
> But you did agreed that the code in question should stay in CSSValuesImpl.
> 
> From you last email in that thread:
> 
> "I agree with Justin on this. I don’t see why numeric font weights is 
> different than backgroundImage for example. Not every app uses 
> backgroundImage, but it should work if used. The same should be for numeric 
> font weights considering fontWeight is a supported style.
> 
> If anything, I could hear an argument that the simplest implementation should 
> be even simpler than it currently is and only handle strings which could be 
> used without conversion. The “simplest useful implementation” should contain 
> Justin’s fix.”
> 
> Serval other people also agreed in that thread the code should stay in that 
> class as it’s fixing a bug with an existing style rather than adding new 
> functionallity.
> 
> Greg did have a suggestion that involved renaming the existing class and 
> breaking it up but no has got around to doing that yet.
> 
> Thanks,
> Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Justin Mclean]

Hi,

But you did agreed that the code in question should stay in CSSValuesImpl.

From you last email in that thread:

"I agree with Justin on this. I don’t see why numeric font weights is different 
than backgroundImage for example. Not every app uses backgroundImage, but it 
should work if used. The same should be for numeric font weights considering 
fontWeight is a supported style.

If anything, I could hear an argument that the simplest implementation should 
be even simpler than it currently is and only handle strings which could be 
used without conversion. The “simplest useful implementation” should contain 
Justin’s fix.”

Serval other people also agreed in that thread the code should stay in that 
class as it’s fixing a bug with an existing style rather than adding new 
functionallity.

Greg did have a suggestion that involved renaming the existing class and 
breaking it up but no has got around to doing that yet.

Thanks,
Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Harbs]

And it was about AllCSSStyles and not AllCSSValuesImpl. I see no reason to not 
use that one.

> On Jun 29, 2017, at 10:14 AM, Harbs  wrote:
> 
> My suggestion was to copy it and prune the ones that are not used.
> 
>> On Jun 29, 2017, at 9:49 AM, Justin Mclean  wrote:
>> 
>> Hi,
>> 
>>> AllCSSValuesImpl - is not enough performant cause of lack of strict 
>>> comparison ?
>> 
>> Just the large number of styles.When Harbs added it he said "It has a LOT of 
>> styles, so it’s probably best to not use in as-is in deployed apps…”
>> 
>> Thanks,
>> Justin
> 

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Harbs]

My suggestion was to copy it and prune the ones that are not used.

> On Jun 29, 2017, at 9:49 AM, Justin Mclean  wrote:
> 
> Hi,
> 
>> AllCSSValuesImpl - is not enough performant cause of lack of strict 
>> comparison ?
> 
> Just the large number of styles.When Harbs added it he said "It has a LOT of 
> styles, so it’s probably best to not use in as-is in deployed apps…”
> 
> Thanks,
> Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Justin Mclean]

Hi,

> AllCSSValuesImpl - is not enough performant cause of lack of strict 
> comparison ?

Just the large number of styles.When Harbs added it he said "It has a LOT of 
styles, so it’s probably best to not use in as-is in deployed apps…”

Thanks,
Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[piotrz]

Hi Justin,

AllCSSValuesImpl - is not enough performant cause of lack of strict
comparison ?

Thanks,
Piotr



-
Apache Flex PMC
piotrzarzyck...@gmail.com
--
View this message in context: 
http://apache-flex-development.247.n4.nabble.com/Re-git-commit-flex-asjs-refs-heads-develop-Fix-merge-error-removing-support-for-numeric-font-size-tp62667p62669.html
Sent from the Apache Flex Development mailing list archive at Nabble.com.

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Justin Mclean]

Hi,

> That's not a merge error.  That code is now in AllCSSValuesImpl and is not
> needed in SimpleCSSValuesImpl.  Please follow the commits.

Consensus was to leave it in SimpleCSSValuesImpl why did you change it without 
discussion? We have a live application that is using this.

AllCSSValuesImpl is not performant and is unlikely to be used in a real 
application so I don’t see much point in moving it there.

Thanks,
Justin

Re: git commit: [flex-asjs] [refs/heads/develop] - Fix merge error removing support for numeric font size.

[Alex Harui]

That's not a merge error.  That code is now in AllCSSValuesImpl and is not
needed in SimpleCSSValuesImpl.  Please follow the commits.

Thanks,
-Alex

On 6/28/17, 11:20 PM, "jmcl...@apache.org"  wrote:

>Repository: flex-asjs
>Updated Branches:
>  refs/heads/develop 18992c5fd -> 68f118b1a
>
>
>Fix merge error removing support for numeric font size.
>
>
>Project: 
>https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit-wip-us
>.apache.org%2Frepos%2Fasf%2Fflex-asjs%2Frepo=02%7C01%7C%7C54ae6d625dc
>94c6ed96e08d4beb6f83f%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C6363431
>40434742994=ajuLAIRntJH18PxMYM%2BlQhhtC%2BaVBA8uA0rJY9nP1%2Fc%3D
>erved=0
>Commit: 
>https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit-wip-us
>.apache.org%2Frepos%2Fasf%2Fflex-asjs%2Fcommit%2F68f118b1=02%7C01%7C%
>7C54ae6d625dc94c6ed96e08d4beb6f83f%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%
>7C0%7C636343140434742994=7%2Bww%2FhSm8sxGI6MNf8l2RfBbK6tN9FrpR7kQthT
>tHtg%3D=0
>Tree: 
>https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit-wip-us
>.apache.org%2Frepos%2Fasf%2Fflex-asjs%2Ftree%2F68f118b1=02%7C01%7C%7C
>54ae6d625dc94c6ed96e08d4beb6f83f%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C
>0%7C636343140434742994=BTgGfk7XE9PNBIbDm8%2BIbpghN1Orxl2krDE2UUTlugU
>%3D=0
>Diff: 
>https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit-wip-us
>.apache.org%2Frepos%2Fasf%2Fflex-asjs%2Fdiff%2F68f118b1=02%7C01%7C%7C
>54ae6d625dc94c6ed96e08d4beb6f83f%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C
>0%7C636343140434742994=VqBJ4z1SXLo40Lt8URgDUyxBAbQd7s5ilAu4LfQSLGw%3
>D=0
>
>Branch: refs/heads/develop
>Commit: 68f118b1a56a49484c4e898826af4834c0cc2fa2
>Parents: 18992c5
>Author: Justin Mclean 
>Authored: Thu Jun 29 16:20:17 2017 +1000
>Committer: Justin Mclean 
>Committed: Thu Jun 29 16:20:17 2017 +1000
>
>--
> .../flex/org/apache/flex/core/SimpleCSSValuesImpl.as | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>--
>
>
>https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit-wip-us
>.apache.org%2Frepos%2Fasf%2Fflex-asjs%2Fblob%2F68f118b1%2Fframeworks%2Fpro
>jects%2FCore%2Fsrc%2Fmain%2Fflex%2Forg%2Fapache%2Fflex%2Fcore%2FSimpleCSSV
>aluesImpl.as=02%7C01%7C%7C54ae6d625dc94c6ed96e08d4beb6f83f%7Cfa7b1b5a
>7b34438794aed2c178decee1%7C0%7C0%7C636343140434742994=vCMPxmTgZtYhvN
>rmc9lf55fRtcJU%2B4%2F1QWADDR4qH7s%3D=0
>--
>diff --git 
>a/frameworks/projects/Core/src/main/flex/org/apache/flex/core/SimpleCSSVal
>uesImpl.as 
>b/frameworks/projects/Core/src/main/flex/org/apache/flex/core/SimpleCSSVal
>uesImpl.as
>index f5d71ca..16b42c6 100644
>--- 
>a/frameworks/projects/Core/src/main/flex/org/apache/flex/core/SimpleCSSVal
>uesImpl.as
>+++ 
>b/frameworks/projects/Core/src/main/flex/org/apache/flex/core/SimpleCSSVal
>uesImpl.as
>@@ -764,7 +764,13 @@ package org.apache.flex.core
> 'constructor': 1
> };
> 
>-
>+/**
>+ * The styles that can use raw numbers
>+ */
>+COMPILE::JS
>+public static const numericStyles:Object = {
>+'fontWeight': 1
>+};
> 
> /**
>  * @param thisObject The object to apply styles to;
>@@ -777,6 +783,7 @@ package org.apache.flex.core
> var styleList:Object = SimpleCSSValuesImpl.perInstanceStyles;
> var colorStyles:Object = SimpleCSSValuesImpl.colorStyles;
> var skipStyles:Object = SimpleCSSValuesImpl.skipStyles;
>+var numericStyles:Object = SimpleCSSValuesImpl.numericStyles;
> var listObj:Object = styles;
> if (styles.styleList)
> listObj = styles.styleList;
>@@ -791,6 +798,8 @@ package org.apache.flex.core
> if (typeof(value) === 'number') {
> if (colorStyles[p])
> value = CSSUtils.attributeFromColor(value);
>+else if (numericStyles[p])
>+value = value.toString();
> else
> value = value.toString() + 'px';
> }
>