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

Add literal support for formatting masks #17

Merged
merged 3 commits into from Aug 13, 2016

Conversation

amoilanen
Copy link
Contributor

It would be nice to be able to use literals in formatting masks, for example

fecha.format(new Date(2001, 2, 5, 6, 7, 2, 5), '[on] MM-DD-YYYY [at] HH:mm'); 
// 'on 03-05-2001 at 06:07'

moment.js includes support for literals as well http://stackoverflow.com/questions/28241002/moment-js-include-text-in-middle-of-date-format

This PR adds support for literals in formatting masks, updates README to include an example of literal usage and increases the minor version number.

@taylorhakes
Copy link
Owner

Thanks for creating the pull request. Just a couple of changes.

//Make literals inactive by replacing them with _
mask = mask.replace(literal, function($0, $1) {
literals.push($1);
return '_';
Copy link
Owner

Choose a reason for hiding this comment

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

Can we use something more unlikely to be used? Someone may put a _ and break this. How about ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will update _ to ??

- Use another placeholder when handling literals
- Format comments
- Update package version to the original version
@amoilanen
Copy link
Contributor Author

Ported tests from moment https://github.com/moment/moment/blob/develop/src/test/moment/format.js#L12 except for the ones that deal with localized tokens.

One of the ported tests revealed an issue with processing literals that span multiple lines, fixed it.

@taylorhakes
Copy link
Owner

This is great. I really appreciate you doing this.

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.

None yet

2 participants