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

$filter number: Could we display an Infinity symbol in case of Infinity value? #10421

Closed
QuentinFchx opened this Issue Dec 11, 2014 · 13 comments

Comments

Projects
None yet
6 participants
@QuentinFchx
Contributor

QuentinFchx commented Dec 11, 2014

Could we display an Infinity symbol (∞) instead of an empty string through the "number" filter, when it's applied to Infinity/-Infinity?
According to this thread, we could use ∞ or ∞ (decimal) or ∞ (hex)

I can come with a PR if it's in the framework's scope. I guess we only have to add a case here.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 11, 2014

Contributor

I wonder if CLDR includes localized "infinity symbols", hah. @petebacondarwin wdyt? Doesn't seem like a big deal to me (I would label this as "Good First Bug" if we had the label already)

Contributor

caitp commented Dec 11, 2014

I wonder if CLDR includes localized "infinity symbols", hah. @petebacondarwin wdyt? Doesn't seem like a big deal to me (I would label this as "Good First Bug" if we had the label already)

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 11, 2014

Contributor

@QuentinFchx you can submit a PR, I'll be happy to review it, it doesn't seem unreasonable and it's pretty low impact so go for it.

It should be pretty obvious what to do --- check if abs(number) is Infinity, and if it is, return a symbol (if CLDR does actually localize this, we should use a localized version if available, but I doubt it does). Then just add some tests demonstrating that it does work correctly. Should take all of 10 minutes to write up.

Contributor

caitp commented Dec 11, 2014

@QuentinFchx you can submit a PR, I'll be happy to review it, it doesn't seem unreasonable and it's pretty low impact so go for it.

It should be pretty obvious what to do --- check if abs(number) is Infinity, and if it is, return a symbol (if CLDR does actually localize this, we should use a localized version if available, but I doubt it does). Then just add some tests demonstrating that it does work correctly. Should take all of 10 minutes to write up.

@QuentinFchx

This comment has been minimized.

Show comment
Hide comment
@QuentinFchx

QuentinFchx Dec 11, 2014

Contributor

Thanks for your quick answer !

I started to dive into the filter's code.
Should I add this feature into the directive's code or into the format function that is shared with the currency directive ?
Modifying the format function will avoid code duplication (especially to handle the negative infinity), but we'll have to inject $locale to retrieve the Infinity symbol. Might that cause any issue ?

Also, there is no Infinity symbol in the locales. Should we add it there aswell ?

Contributor

QuentinFchx commented Dec 11, 2014

Thanks for your quick answer !

I started to dive into the filter's code.
Should I add this feature into the directive's code or into the format function that is shared with the currency directive ?
Modifying the format function will avoid code duplication (especially to handle the negative infinity), but we'll have to inject $locale to retrieve the Infinity symbol. Might that cause any issue ?

Also, there is no Infinity symbol in the locales. Should we add it there aswell ?

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 11, 2014

Contributor

I'm half-joking about localizing the infinity symbol --- I expect there's no CLDR data for localized Infinity. If someone proves me wrong then sure, but that's a bigger project than this and it's not worth going for it just for this

Contributor

caitp commented Dec 11, 2014

I'm half-joking about localizing the infinity symbol --- I expect there's no CLDR data for localized Infinity. If someone proves me wrong then sure, but that's a bigger project than this and it's not worth going for it just for this

@QuentinFchx

This comment has been minimized.

Show comment
Hide comment
@QuentinFchx

QuentinFchx Dec 11, 2014

Contributor

Might this line kill Buzz Lightyear dreams (and mines too ?) then ?

Contributor

QuentinFchx commented Dec 11, 2014

Might this line kill Buzz Lightyear dreams (and mines too ?) then ?

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 11, 2014

Contributor

well if they're getting that from CLDR then we probably do want to add that --- but for this patch I wouldn't worry about it.

Contributor

caitp commented Dec 11, 2014

well if they're getting that from CLDR then we probably do want to add that --- but for this patch I wouldn't worry about it.

@QuentinFchx

This comment has been minimized.

Show comment
Hide comment
@QuentinFchx

QuentinFchx Dec 11, 2014

Contributor

Fine !
Do we still agree on modifying the format function and not the directive's code to avoid code duplication (again, meaning that the currency filter will be impacted) ?

Contributor

QuentinFchx commented Dec 11, 2014

Fine !
Do we still agree on modifying the format function and not the directive's code to avoid code duplication (again, meaning that the currency filter will be impacted) ?

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 11, 2014

Contributor

I never agreed on that in the first place, but do it how you like

Contributor

caitp commented Dec 11, 2014

I never agreed on that in the first place, but do it how you like

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Dec 17, 2014

Member

It seems like the associated PR was moved to 1.4.x so moving the issue as well.

It seems like the associated PR was moved to 1.4.x so moving the issue as well.

@pkozlowski-opensource pkozlowski-opensource modified the milestones: 1.4.x, 1.3.8 Dec 17, 2014

@lgalfaso

This comment has been minimized.

Show comment
Hide comment
@lgalfaso

lgalfaso Jan 8, 2015

Member

landed as #10422

Member

lgalfaso commented Jan 8, 2015

landed as #10422

@lgalfaso lgalfaso closed this Jan 8, 2015

lgalfaso added a commit that referenced this issue Jan 8, 2015

feat($filter): Display Infinity symbol when number is Infinity
Infinity is a value and should not be treated as an empty string

Closes #10421
@leodido

This comment has been minimized.

Show comment
Hide comment
@leodido

leodido Mar 27, 2015

I think this issue is not yet completely fixed.

I explain myself with some bits of code:

  Infinity (1): <span>{{1/0 | number:4}}</span><br> 
  Infinity (2): <span>{{Infinity | number:4}}</span><br>
  Infinity (3): <span>{{'Infinity' | number:4}}</span><br>
  • cases (1) and (3) work as expected: they both print .
  • case (2) does not work, it prints an empty string because number parameter passed to the filter has undefined value.

Please note infact that calling numberFilter(Infinity, 4) the filtering works, returning infinite symbol.

Therefore I believe that most likely the problem resides in the expression evaluation.

leodido commented Mar 27, 2015

I think this issue is not yet completely fixed.

I explain myself with some bits of code:

  Infinity (1): <span>{{1/0 | number:4}}</span><br> 
  Infinity (2): <span>{{Infinity | number:4}}</span><br>
  Infinity (3): <span>{{'Infinity' | number:4}}</span><br>
  • cases (1) and (3) work as expected: they both print .
  • case (2) does not work, it prints an empty string because number parameter passed to the filter has undefined value.

Please note infact that calling numberFilter(Infinity, 4) the filtering works, returning infinite symbol.

Therefore I believe that most likely the problem resides in the expression evaluation.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Mar 27, 2015

Member

I think it works as expected.

<span>{{Infinity | number:4}}</span> is equivalent to numberFilter($scope.Infinity, 4), which is undefined (unless explicitly defined by the user).

Keep in mind, that JS expressions in the controller are evaluated against the global object, whereas Angular expressions in the view are evaluated against the local $scope.

Member

gkalpak commented Mar 27, 2015

I think it works as expected.

<span>{{Infinity | number:4}}</span> is equivalent to numberFilter($scope.Infinity, 4), which is undefined (unless explicitly defined by the user).

Keep in mind, that JS expressions in the controller are evaluated against the global object, whereas Angular expressions in the view are evaluated against the local $scope.

@leodido

This comment has been minimized.

Show comment
Hide comment
@leodido

leodido Mar 27, 2015

Okay, I get it. Sorry and thanks.

leodido commented Mar 27, 2015

Okay, I get it. Sorry and thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment