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

Fix 213 Number: Integer and fraction digits format #209

Closed
wants to merge 2 commits into from
Closed

Conversation

rxaviers
Copy link
Member

Implements #213

"cldr",
"../globalize"
], factory );
} else if ( typeof module === "object" && typeof module.exports === "object" ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What UMD version is this based on? Neither current returnExports nor commonjsStrict use this check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Related discussion: qunitjs/qunit#540

Copy link
Member Author

Choose a reason for hiding this comment

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

It's based on https://github.com/jquery/jquery/blob/6de1d973a436282af7dfe3072924016c3cdc1984/src/intro.js, but skipping their window concerns. But, to be honest I didn't investigate why Core is following this approach instead of any of the UMDs you pointed out above.

On qunitjs/qunit#540, where can I find more info regarding "From previous chats with @jdalton, I believe there are a few more variations necessary to thoroughly export for all CommonJS environments, none of which require a nodeType check AFAIR"?

Copy link

Choose a reason for hiding this comment

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

The gist is

  if (checkForAMDFirst) {
    // amd define stuff
  }
  else if (freeExports && freeModule) {
    if (moduleExports) {
      // the reason you bolt `myFunc` onto `myFunc` is for
      // consistent use in environments without `module.exports`
      (freeModule.exports = myFunc).myFunc = ,myFunc;
    }
    else {
      freeExports.myFunc = myFunc;
    }
  }
  else {
    // in a browser
    root.myFunc = myFunc;
  }

The detection and assignment of checkForAMDFirst, freeExports, freeModule, moduleExports will vary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @jdalton.

@jzaefferer, which UMD wrapper do you think we should use?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need something that we can test. There's some progress in that regard in here, though its still far from done: https://github.com/jquery/qunit/pull/540/files

I think for now our best bet is to stick with an appropriate UMD wrapper and let them figure out the compatibility issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. So, I'm picking the returnExports flavor, and I've opened issue #221 to keep track of this change.

@jzaefferer
Copy link
Contributor

Took me a while to figure out that I have to both npm install and bower install, eventually found it at the end of the readme. Maybe integrate grunt-bowercopy (using that in UI now)?

@rxaviers rxaviers changed the title Create Number module Fix 213 Number: Integer and fraction digits format Mar 21, 2014
@rxaviers
Copy link
Member Author

Took me a while to figure out that I have to both npm install and bower install, eventually found it at the end of the readme. Maybe integrate grunt-bowercopy (using that in UI now)?

@jzaefferer, I agree that triggering bower install from the Gruntfile saves the one time execution of such command. But, in the other hand, it keeps executing it (most of the times unnecessarily) on every build during development. If the case, can we make it a separate issue please?

@scottgonzalez
Copy link
Contributor

The bowercopy command wouldn't be part of the build. It'd be a standalone command that you would run manually.

@rxaviers
Copy link
Member Author

What's the gain then by replacing bower install for grunt <something>?

@jzaefferer
Copy link
Contributor

On UI we use that to update the files in external, which are committed to the repo, so there's no need to do that for every clone of the repository. With bowercopy you can specify exactly which files you want, while bower itself usually copies a bunch of unnecessary files.

I don't know how useful that will be here though, it was a suggestion, not a requirement.

@rxaviers
Copy link
Member Author

Ok. Thanks.

rxaviers added a commit that referenced this pull request Mar 26, 2014
Implement integer and fraction digits format

Allow Globalize.format() to override locale for the following options:
- Minimum integer digits `minimumIntegerDigits`;
- Minimum fraction digits `minimumFractionDigits`;
- Maximum fraction digits `maximumFractionDigits`;

Number format setup is performed by numberFormatProperties(). Number
format execution is performed by numberFormat(). This arrangement allows
subsequent setup caching for improved performance. Yet this
implementation doesn't follow ECMA-402, it's been based on
Intl.NumberFormat instance properties.

It accepts the following `round` options:
- "ceil";
- "floor";
- "round" (default);
- "truncate";

Ref gh-200
Closes gh-209, gh-213
rxaviers added a commit that referenced this pull request Mar 26, 2014
Number format setup is performed by numberFormatProperties(). Number
format execution is performed by numberFormat(). This arrangement allows
subsequent setup caching for improved performance. Yet this
implementation doesn't follow ECMA-402, it's been based on
Intl.NumberFormat instance properties.

Implement integer and fraction digits format

Allow the following integer and fraction override options:
- Minimum integer digits `minimumIntegerDigits`;
- Minimum fraction digits `minimumFractionDigits`;
- Maximum fraction digits `maximumFractionDigits`;

It accepts the following `round` options:
- "ceil";
- "floor";
- "round" (default);
- "truncate";

Ref gh-200
Closes gh-209, gh-213
rxaviers added a commit that referenced this pull request Mar 26, 2014
Number format setup is performed by numberFormatProperties(). Number
format execution is performed by numberFormat(). This arrangement allows
subsequent setup caching for improved performance. Yet this
implementation doesn't follow ECMA-402, it's been based on
Intl.NumberFormat instance properties.

Implement integer and fraction digits format.

Allow the following integer and fraction override options:
- Minimum integer digits `minimumIntegerDigits`;
- Minimum fraction digits `minimumFractionDigits`;
- Maximum fraction digits `maximumFractionDigits`;

It accepts the following `round` options:
- "ceil";
- "floor";
- "round" (default);
- "truncate";

Ref gh-200
Closes gh-209, gh-213
Create initial number module.

Number format setup is performed by numberFormatProperties(). Number
format execution is performed by numberFormat(). This arrangement allows
subsequent setup caching for improved performance. Yet this
implementation doesn't follow ECMA-402, it's been based on
Intl.NumberFormat instance properties.

Implement integer and fraction digits format.

Allow the following integer and fraction override options:
- Minimum integer digits `minimumIntegerDigits`;
- Minimum fraction digits `minimumFractionDigits`;
- Maximum fraction digits `maximumFractionDigits`;

It accepts the following `round` options:
- "ceil";
- "floor";
- "round" (default);
- "truncate";

Ref gh-200
Closes gh-209, gh-213
@rxaviers rxaviers closed this in 98a851b Mar 28, 2014
@rxaviers rxaviers mentioned this pull request Jun 9, 2014
1 task
@rxaviers rxaviers deleted the number branch August 25, 2014 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants