Add short number format #1

snewcomer · 2018-10-09T21:17:02Z

Ref PRs:
snewcomer/formatjs-extract-cldr-data#1
ember-intl/intl-messageformat-parser#1

snewcomer · 2018-10-12T15:13:29Z

@jasonmit Having some trouble testing my branch in this PR (or just master) as I need to pull in my PR to add shortNumber type. Looking for some help! 🙏

ember-intl/intl-messageformat-parser#1

Current intl-messageformat-parser - v1.4.0

sn/add-type or master

So the current version for intl-messageformat-parser produces a dist folder and src/parser.js file. When pointing to my PR or even master, I think there is some build step I am missing. Do you happen to know?

jasonmit · 2018-10-12T17:52:44Z

I'm not sure I'm understanding the issue, if I pull this branch down and try and build I'll see this issue?

Are you yarn/npm linking @ember-intl/intl-messageformat? If so, you'll need to grunt to build the dist as far as I know. Let me know if that doesn't resolve your issue.

snewcomer · 2018-10-12T20:12:22Z

Basically on master of this lib, npm install (also grunt via ‘prepublish’) produces a ‘src/parser.js’ file under the intl-messageformat-parser bundle (under ‘node_modules’). When pointing to my PR, I believe there is a build step that is missing. You’ll see under the intl-messageformat-parser bundle, the only file is ‘src/parser.pegjs’ && as a result the import in this library fails bc it cant find the file.

So yeah if you pull down and point the parser dep to my PR, you will see the failed import. I’ll try to get a better explanation up when I get to a computer!!

snewcomer · 2018-10-12T23:15:51Z

Perhaps a versioned release of the parser might solve the issue as well.

jasonmit · 2018-10-14T08:15:00Z

Perhaps a versioned release of the parser might solve the issue as well.

It is versioned of npm, unsure what is happening here. Will need to look into it.

jasonmit · 2018-10-14T08:30:48Z

src/compiler.js

    return options[value] || options.other;
 };
+
+function ShortNumberFormat(locales, options) {


Could be its own lib so changes to it are not necessarily coupled to the compiler. Can be a future PR.

For now, I'd move into its own module though.

Do you still want this in it's own module now that much of the work is in cldr-compact-number?

No this seems fine for now

jasonmit · 2018-10-14T08:31:52Z

src/compiler.js

+  // 3. Normalise number by converting to decimal and cropping to number of digits
+  // 22 -> 22
+  // 1000 -> 1.000 -> 1K
+  // 1600 -> 1.600 -> 2K


Is it possible to support rounding so it doesn't round to the nearly whole number? I could see someone wanting this to render 1.6k

@jasonmit Totally! So I put up an addon as not only a proof of concept for this, but also for apps that are still on ember-i18n.

I'll try to see what changes should be brought here; which contains logic to support 1.6k.

https://github.com/snewcomer/ember-short-number

Love it! How difficult would it be to have us add ember-short-number as a dep to ember-intl? This way you don't have to re-implement similar logic in two places!

Since we are using the ICU message syntax for this, I'm not certain how easy that would be. I like the idea! Might just have to explore a little more though.

I have {numPeople, shortNumber}

https://github.com/ember-intl/intl-messageformat#user-defined-formats could be an option. It's how we provide options to the other formatters that accept them.

Looks like I need to also allow options { significantDigits: 1 } to be passed into shortNumber. I'll try to figure that out this week with the user-defined-formats you mentioned.

todo · 2018-11-04T17:52:03Z

loop through and find first match

intl-messageformat/src/compiler.js

Lines 136 to 141 in 092f2be

    
               optionsHash = {}; 
        
           // Save the current plural element, if any, then set it to a new value when 
        
           // compiling the options sub-patterns. This conforms the spec's algorithm 
        
           // for handling `"#"` syntax in message text. 
        
           this.pluralStack.push(this.currentPlural);

This comment was generated by todo based on a `TODO` comment in `092f2be` in #1. cc @snewcomer.

snewcomer · 2018-11-16T15:57:13Z

@jasonmit I made some good progress here locally. One last step to get the tests passing - Should we move fork this lib to ember-intl? Need to upgrade a dependency. https://github.com/yahoo/grunt-extract-cldr-data

Only issue I see with forking is no issues section on the @ember-intl repo, but perhaps that isn't too big of a deal.

jasonmit · 2018-11-16T22:45:30Z

@snewcomer https://github.com/ember-intl/grunt-extract-cldr-data I forked for you. I also will update our forked repos to open up issues.

snewcomer

@jasonmit Got the tests passing! Have a few comments. Lmk what you think!

snewcomer · 2018-11-26T21:58:25Z

src/compiler.js


 /* jslint esnext: true */

+import * as CompactFormat from 'cldr-compact-number';


I'm having, what I believe to be, interop issues with cjs and esm. Essentially if I import CompactFormat from 'cldr-compact-number', it says cldrCompactNumber.default` is not a function.

I added this line for jsnext entry point. CompactFormat is just a function, but rollup (??) may be causing an issue. Any ideas?

webpack/webpack#7973 (comment)

On a related note, we could rewrite this lib depending on your thoughts on it are. Perhaps typescript would be a good format.

@snewcomer I'll have a look at this over the weekend

Is this still a blocker @snewcomer ?

As of right now, I don't think so. As a namespace import, there is only one default export anyways - compactFormat.

I expected this to work, but doesn't.

import compactFormat from 'cldr-compact-number'; // TypeError: cldr$compact$number$$.default is not a function

snewcomer · 2018-11-26T21:58:56Z

src/compiler.js

    return options[value] || options.other;
 };
+
+function ShortNumberFormat(locales, options) {


Do you still want this in it's own module now that much of the work is in cldr-compact-number?

snewcomer · 2018-11-26T22:03:50Z

.gitignore

@@ -1,3 +1,4 @@
+dist/locale-data


After looking through everything, I don't think we need to publish the locale-data/ contents to git nor npm. What do you think?

If you agree, I will:

git rm -r --cached . git add . git commit -m "fixed untracked files"

Looked at this one more time and believe these aren't needed up in git or npm. I'll wait for confirmation before removing from this PR though 👍

Unlikely they need to be in git, but they should be published to npm (so we can upstream these changes back into intl-messageformat). ember-intl is not dependent on this disted locale data though!

@jasonmit Ok. Removed from .git (that is what you wanted me to do right?)

snewcomer · 2018-12-29T18:59:42Z

src/compiler.js

            };

+        case 'shortNumberFormat':
+            options = formats.number;


@jasonmit my last question is - should we be using the User Defined Formats here?

var msg = new IntlMessageFormat('My short number: {numPeople, shortNumber, options}', 'en-US', { number: { options: { significantDigits : 1, financialFormat: true } } });

or is it better/simpler as what currently exists in this PR (does this even work in ember-intl?):

var msg = new IntlMessageFormat('My short number: {numPeople, shortNumber}', 'en-US', { number: { significantDigits : 1, financialFormat: true } });

or is it better/simpler as what currently exists in this PR (does this even work in ember-intl?):

The first example allows you to pass options to each segment and the latter would just be universal for the whole translation.

I think if we're going to support one of these it should be the first one where we can pass the format hash key. This way it allows a translation to contain different formatting options for each shortNumber.

var msg = new IntlMessageFormat('My short number: {value, shortNumber, fin}', 'en-US', { shortNumber: { // <- we should not use number either fin: { significantDigits : 1, financialFormat: true } } });

@jasonmit What do you think about this? 30a1159

Then in ember-intl, this will work right? --
people: '{numPeople, shortNumber, options} will be there!'

const formats = { shortNumber: { options: { minimumFractionDigits: 2 } } }; this.get('intl').t('people', { numPeople: 1999 }, formats);

@jasonmit What do you think about this? 30a1159

Looks good but can we remove the need to provide options? Something like this would align well with the way we define other formatter options:

const formats = { shortNumber: { minimumFractionDigits: 2 } };

(Let me know if I'm overlooking something.)

snewcomer · 2019-01-08T16:17:54Z

tests/index.js

+        shortNumber: {
+            significantDigits: 1
+        }
+      });


@jasonmit How it would work in ember-intl. Lmk what you think. I believe this PR good to go but would love some more feedback if need be!

this.intl.t('people', { numPeople: 1200 }, { shortNumber: { significantDigits: 1 } } )

this.intl.t('people.number', { numPeople: 1200 }, { shortNumber: { significantDigits: 1 } } )

isn't something that would work out of the box today. t() takes two args: the translation key and the options. I think the formatter options shortNumber would need to be set via named formats in ember-intl land vs supplying a 3rd argument.

See my comment below which will work out of the box: #1 (comment)

@jasonmit 😮 I was basing off this section in the docs with the 3rd arg as optionalFormats:Object. I thought this would work but haven't verified manually.

people: '{numPeople, shortNumber} will be attending.'

If this doesn't work, lmk how I can set via named formats in ember-intl land. I should build this branch and test out in ember-intl directly sometime this week.

Yikes, I'll update the docs to remove the 3rd arg since it does not exist. Sorry for the troll.

lmk how I can set via named formats in ember-intl land

Example of defining a named format:
https://github.com/ember-intl/ember-intl/blob/master/tests/dummy/app/formats.js

Given a translation like "{activeUsers, shortNumber, aNamedFormat}" this should be how you would define the formatter options in formats.js

// app/format.js export default { shortNumber: { aNamedFormat: { significantDigits: 1 } } };

I can implement the integration with ember-intl once #1 (comment) is addressed

Edit: I updated the README to remove the old API references.

Those are per user defined right? Like what needs to happen on the ember-intl side?

Those are per user defined right? Like what needs to happen on the ember-intl side?

Exactly

jasonmit · 2019-01-08T19:27:42Z

tests/index.js

+
+  describe('and short number formatting with options', function () {
+      var msg = '' +
+          'I have {numPeople, shortNumber, options}';


Confused by the use of options here. This would align with how the other named formats are provided:

var msg = '{activeUsers, shortNumber, aNamedFormat} of {totalUsers, shortNumber, anotherNamedFormat}'; var msgFmt = new IntlMessageFormat(msg, 'en-US', { anotherNamedFormat: { significantDigits: 0 }, aNamedFormat: { significantDigits: 1 } });

Oh you are right. I don't think options is needed. Let me test and push up.

@jasonmit Alrighty lmk what you think about this commit! aaa4fe9

Looks good!

snewcomer · 2019-01-11T17:45:21Z

@jasonmit 👋 Was there anything you needed from me? I wasn't sure what you were thinking of in terms of next steps...

jasonmit · 2019-01-11T19:11:50Z

@snewcomer I don't believe so. The only part left is to integrate with ember-intl.

I was planning to tackle this, just cannot commit to a timeline (really busy ATM). If you have the cycles, feel free to take that portion on.

snewcomer · 2019-01-11T19:44:52Z

Ah ok. Wasn't sure if you were ok with this going into master. Or if I should wait for an approval...I got time fyi

jasonmit · 2019-01-11T21:23:54Z

@snewcomer feel free to merge :) If you wanted to start tackling ember-intl integration feel free. As always, ping me if you need anything!

snewcomer · 2019-01-13T16:16:44Z

Turns out the default import issue was an issue with UMD and tests. UMD + ember-intl worked. CJS and tests worked. So instead of default export compactNumber, I just did a named export and that allowed the tests to pass in #7. I've seen a few issues across the interwebs with default exports in various scenarios. 🤷‍♂️ PR for ember-intl in progress!

snewcomer force-pushed the sn/short-number branch from 12cc2cc to f3c65ac Compare October 10, 2018 15:55

snewcomer mentioned this pull request Oct 12, 2018

Add shortNumber type ember-intl/intl-messageformat-parser#1

Merged

jasonmit reviewed Oct 14, 2018

View reviewed changes

snewcomer self-assigned this Nov 4, 2018

snewcomer added the enhancement New feature or request label Nov 4, 2018

snewcomer mentioned this pull request Nov 4, 2018

Update intl-messageformat-parser to 1.5.0 ember-intl/ember-intl#666

Merged

snewcomer mentioned this pull request Nov 16, 2018

Update formatjs to 6.0 under ember-intl org ember-intl/grunt-extract-cldr-data#1

Merged

snewcomer force-pushed the sn/short-number branch from 6962ea9 to eeadb9f Compare November 19, 2018 03:36

This was referenced Nov 20, 2018

Add default export snewcomer/cldr-compact-number#1

Merged

Add number fields and dist #3

Merged

snewcomer added 11 commits November 26, 2018 10:28

Add short number format

a696bed

get working on extracting and using rules to format

9a3fda1

Update compiler

0b42882

no interpolation strings

afdd9d9

fix errorg

94c01e8

cleanup

71793e9

update some urls in pkg

e693899

add moare tsts cases

0cc4c51

point to master

a9cb4a9

update parser to 1.5.0

8ed16c1

install cldr-compact-number

035a715

snewcomer commented Nov 26, 2018

View reviewed changes

fixed untracked files

76d91c1

ember-intl deleted a comment from todo bot Nov 30, 2018

snewcomer added 2 commits December 29, 2018 10:41

add tests for significant digits and other language

bc142a7

update dist

ef639c2

snewcomer commented Dec 29, 2018

View reviewed changes

snewcomer added 6 commits December 29, 2018 15:58

add "shortNumber" format

3832caf

dynamic segment for options to short number formatter

30a1159

update dist

6551ba7

remove options

93ef977

udpate dist

a3a1e51

camelCase namespace import

652461e

snewcomer commented Jan 8, 2019

View reviewed changes

jasonmit reviewed Jan 8, 2019

View reviewed changes

snewcomer added 4 commits January 8, 2019 11:47

no need for "options" in translation key

62951ea

Fix based on named formats in ember-intl

aaa4fe9

new dist

4021c34

fix indenting issue in test

65a9dd2

jasonmit approved these changes Jan 11, 2019

View reviewed changes

snewcomer merged commit 39cc768 into ember-intl:master Jan 11, 2019

This was referenced Jan 11, 2019

Import IntlMessageFormat #4

Merged

Fix last issues for ember-intl #5

Merged

snewcomer deleted the sn/short-number branch January 13, 2019 16:14

snewcomer mentioned this pull request Mar 12, 2019

Compact Decimal Format messageformat/messageformat#229

Closed


		/* jslint esnext: true */

		import * as CompactFormat from 'cldr-compact-number';

Add short number format #1

Add short number format #1

Uh oh!

Conversation

snewcomer commented Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snewcomer commented Oct 12, 2018

Uh oh!

jasonmit commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snewcomer commented Oct 12, 2018

Uh oh!

snewcomer commented Oct 12, 2018

Uh oh!

jasonmit commented Oct 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

todo bot commented Nov 4, 2018

loop through and find first match

This comment was generated by todo based on a TODO comment in 092f2be in #1. cc @snewcomer.

Uh oh!

snewcomer commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonmit commented Nov 16, 2018

Uh oh!

snewcomer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmit Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmit Dec 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snewcomer commented Oct 9, 2018 •

edited

Loading

jasonmit commented Oct 12, 2018 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `092f2be` in #1. cc @snewcomer.

snewcomer commented Nov 16, 2018 •

edited

Loading

jasonmit Nov 29, 2018 •

edited

Loading

jasonmit Dec 29, 2018 •

edited

Loading

jasonmit Dec 30, 2018 •

edited

Loading

snewcomer Jan 8, 2019 •

edited

Loading

jasonmit Jan 8, 2019 •

edited

Loading

jasonmit Jan 8, 2019 •

edited

Loading

jasonmit Jan 8, 2019 •

edited

Loading