Skip to content

Plural: Support ordinals #416

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

Closed
wants to merge 1 commit into from
Closed

Plural: Support ordinals #416

wants to merge 1 commit into from

Conversation

tniessen
Copy link
Contributor

Fixes #403

>> **type** Optional
>>
>> String `cardinal` (default), or `ordinal`.

Copy link
Member

Choose a reason for hiding this comment

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

Please, could you move options above value? Also, replace >> with > only.

@rxaviers
Copy link
Member

Hi @tniessen, I left a couple of comments above. Great job so far.

@@ -49,7 +74,7 @@ plural( 1 );
// > "other"
```

For comparison:
For comparison (cardinals):
Copy link
Member

Choose a reason for hiding this comment

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

Let's create a similar table for ordinals?

Copy link
Member

Choose a reason for hiding this comment

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

👍

@jzaefferer
Copy link
Contributor

I'd like to see a brief definition for both cardinals and ordinals somewhere in the documentation. More examples would also help.

@jzaefferer jzaefferer mentioned this pull request Mar 16, 2015
@tniessen
Copy link
Contributor Author

@rxaviers @jzaefferer I pushed a commit a few days ago, awaiting reviews.


| | en (English) | ru (Russian) | ar (Arabic) |
| --- | --- | --- | --- |
| `plural( 0 )` | `other` | `other` | `other` |
Copy link
Member

Choose a reason for hiding this comment

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

All table needs the below change.

- plural( 0 )
+ plural( 0, { type: "ordinal" } )

@rxaviers
Copy link
Member

Hi @tniessen, I added a couple of comments. Github doesn't send notifications when new commits are pushed. So, feel free to add a new comment if you have further updates. Thanks so far.

@tniessen
Copy link
Contributor Author

@rxaviers Agree with both, new code is almost identical with your suggestion. Updated as 583ae45.

@rxaviers
Copy link
Member

Thank you @tniessen, one last comment.

@tniessen
Copy link
Contributor Author

@rxaviers Fixed in 7538dec, thank you for reviewing!

cardinal: cldr.supplemental( "plurals-type-cardinal" )
};
MakePlural.rules = {};
MakePlural.rules[type] = cldr.supplemental( "plurals-type-" + type );
Copy link
Member

Choose a reason for hiding this comment

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

spaces MakePlural.rules[ type ] =

@tniessen
Copy link
Contributor Author

@rxaviers My bad, sorry... c47456f

@rxaviers rxaviers closed this in 13b63ff Mar 20, 2015
@rxaviers
Copy link
Member

@tniessen, no problem. Thanks landed 🎉

@tniessen tniessen deleted the fix-403 branch March 20, 2015 15:25
ashensis pushed a commit to ashensis/globalize that referenced this pull request Mar 17, 2016
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.

Support ordinals
4 participants