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(common): titlecase pipe #22600

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@skryvets
Contributor

skryvets commented Mar 6, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix

What is the current behavior?

Current titlecase pipe doesn't work properly for non-latin alphabet and strings with qoutes

Issue Number: 16586

What is the new behavior?

Titlecase pipe works properly for all existing cases plus for non-latin alphabet and strings with qoutes

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

I took already existing implementation from here and added some unit tests to verify broken functionality.

@vicb

The implementation is not correct when the first letter is not a \w

@skryvets

This comment has been minimized.

Contributor

skryvets commented Mar 7, 2018

Hey @vicb!

Thank you for that catch and sorry for missing that.

I found a regex to handle non-latin characters here and modified the solution to look for non-latin character or word as a first letter.

/\w\S*/g became /([^\x00-\x7F]|\w)\S*/g

I also added unit test to handle the case you provided.

@vicb

This comment has been minimized.

Contributor

vicb commented Mar 8, 2018

I think [^\x00-\x7F] is not good enough. There are some unicode whitespaces that would be matched by this.

What about the reply mentioning /\p{L}/u in the link you provided (the transpiled form) ?

@skryvets

This comment has been minimized.

Contributor

skryvets commented Mar 10, 2018

unicode whitespaces that would be matched by this.

Yes, this is correct. After I researched it, I started to understand it a way better.

What about the reply mentioning /\p{L}/u in the link you provided (the transpiled form) ?

Before commiting transpiled form, I wanted to find out whether it will be possible to use /\p{L}/u from ES2018 for this case. I did a research and asked about it. Even with TS support there is no transpilation down to ES5.

As you said, the only solution is to use long ES5 regular expression itself.

I updated the PR with corresponding change.

Since this is a very long string, I'm not really sure how to structure the code properly. I formatted and pushed as I think it should be. Please correct me if I'm wrong.

@@ -51,7 +40,11 @@ export class TitleCasePipe implements PipeTransform {
throw invalidPipeArgumentError(TitleCasePipe, value);
}
return value.split(/\b/g).map(word => titleCaseWord(word)).join('');
const unicodeLetterMatch =

This comment has been minimized.

@vicb

vicb Mar 13, 2018

Contributor

please add a comment on how you come with this regexp (giving links to docs if appropriate)

This comment has been minimized.

@vicb

vicb Mar 13, 2018

Contributor

Should the name be unicodeWordMatch ?

@vicb

This comment has been minimized.

Contributor

vicb commented Mar 13, 2018

LGTM, please add a comment and squash the commits.
Thanks for tackling this long standing issue !

@vicb vicb self-assigned this Mar 13, 2018

@skryvets skryvets force-pushed the skryvets:fix-titlecase-pipe branch from a39e418 to e50f664 Mar 14, 2018

@skryvets

This comment has been minimized.

Contributor

skryvets commented Mar 15, 2018

@vicb,

I forgot to post message here, but I updated this PR (squashed the commits and added explanation comment.)

@@ -51,7 +40,19 @@ export class TitleCasePipe implements PipeTransform {
throw invalidPipeArgumentError(TitleCasePipe, value);
}
return value.split(/\b/g).map(word => titleCaseWord(word)).join('');
/**

This comment has been minimized.

@vicb

vicb Mar 15, 2018

Contributor

Sorry 2 more nits:

Regex below matches any Unicode letter and compatible with ES5. In ES2018 the same result
can be achieved by using /\p{L}/u and also known as Unicode Property Escapes

is not exactly true, should be:

Regex below matches any Unicode *word* [...] /\p{L}\S*/gu

The message is otherwise great and super useful - it took months and many people to get this right ! Thanks for your work on this.

Also please move the const unicodeWordRegexp at file level so that the regexp does not have to be re-compiled each time the method is called.

@vicb

This comment has been minimized.

Contributor

vicb commented Mar 15, 2018

@sergeome I do not pay attention to github notifications, you have better chance on gitter.

Please update the last 2 nits and this will be ready to go.

Many many thanks !

* https://mothereff.in/regexpu#input=var+regex+%3D+/%5Cp%7BL%7D/u%3B&unicodePropertyEscape=1
*/
const unicodeWordMatch =
/(?:[A-Za-z\xAA\xB5\xBA\xC0-\xD6\xD8-\xF6\xF8-\u02C1\u02C6-\u02D1\u02E0-\u02E4\u02EC\u02EE\u0370-\u0374\u0376\u0377\u037A-\u037D\u037F\u0386\u0388-\u038A\u038C\u038E-\u03A1\u03A3-\u03F5\u03F7-\u0481\u048A-\u052F\u0531-\u0556\u0559\u0561-\u0587\u05D0-\u05EA\u05F0-\u05F2\u0620-\u064A\u066E\u066F\u0671-\u06D3\u06D5\u06E5\u06E6\u06EE\u06EF\u06FA-\u06FC\u06FF\u0710\u0712-\u072F\u074D-\u07A5\u07B1\u07CA-\u07EA\u07F4\u07F5\u07FA\u0800-\u0815\u081A\u0824\u0828\u0840-\u0858\u0860-\u086A\u08A0-\u08B4\u08B6-\u08BD\u0904-\u0939\u093D\u0950\u0958-\u0961\u0971-\u0980\u0985-\u098C\u098F\u0990\u0993-\u09A8\u09AA-\u09B0\u09B2\u09B6-\u09B9\u09BD\u09CE\u09DC\u09DD\u09DF-\u09E1\u09F0\u09F1\u09FC\u0A05-\u0A0A\u0A0F\u0A10\u0A13-\u0A28\u0A2A-\u0A30\u0A32\u0A33\u0A35\u0A36\u0A38\u0A39\u0A59-\u0A5C\u0A5E\u0A72-\u0A74\u0A85-\u0A8D\u0A8F-\u0A91\u0A93-\u0AA8\u0AAA-\u0AB0\u0AB2\u0AB3\u0AB5-\u0AB9\u0ABD\u0AD0\u0AE0\u0AE1\u0AF9\u0B05-\u0B0C\u0B0F\u0B10\u0B13-\u0B28\u0B2A-\u0B30\u0B32\u0B33\u0B35-\u0B39\u0B3D\u0B5C\u0B5D\u0B5F-\u0B61\u0B71\u0B83\u0B85-\u0B8A\u0B8E-\u0B90\u0B92-\u0B95\u0B99\u0B9A\u0B9C\u0B9E\u0B9F\u0BA3\u0BA4\u0BA8-\u0BAA\u0BAE-\u0BB9\u0BD0\u0C05-\u0C0C\u0C0E-\u0C10\u0C12-\u0C28\u0C2A-\u0C39\u0C3D\u0C58-\u0C5A\u0C60\u0C61\u0C80\u0C85-\u0C8C\u0C8E-\u0C90\u0C92-\u0CA8\u0CAA-\u0CB3\u0CB5-\u0CB9\u0CBD\u0CDE\u0CE0\u0CE1\u0CF1\u0CF2\u0D05-\u0D0C\u0D0E-\u0D10\u0D12-\u0D3A\u0D3D\u0D4E\u0D54-\u0D56\u0D5F-\u0D61\u0D7A-\u0D7F\u0D85-\u0D96\u0D9A-\u0DB1\u0DB3-\u0DBB\u0DBD\u0DC0-\u0DC6\u0E01-\u0E30\u0E32\u0E33\u0E40-\u0E46\u0E81\u0E82\u0E84\u0E87\u0E88\u0E8A\u0E8D\u0E94-\u0E97\u0E99-\u0E9F\u0EA1-\u0EA3\u0EA5\u0EA7\u0EAA\u0EAB\u0EAD-\u0EB0\u0EB2\u0EB3\u0EBD\u0EC0-\u0EC4\u0EC6\u0EDC-\u0EDF\u0F00\u0F40-\u0F47\u0F49-\u0F6C\u0F88-\u0F8C\u1000-\u102A\u103F\u1050-\u1055\u105A-\u105D\u1061\u1065\u1066\u106E-\u1070\u1075-\u1081\u108E\u10A0-\u10C5\u10C7\u10CD\u10D0-\u10FA\u10FC-\u1248\u124A-\u124D\u1250-\u1256\u1258\u125A-\u125D\u1260-\u1288\u128A-\u128D\u1290-\u12B0\u12B2-\u12B5\u12B8-\u12BE\u12C0\u12C2-\u12C5\u12C8-\u12D6\u12D8-\u1310\u1312-\u1315\u1318-\u135A\u1380-\u138F\u13A0-\u13F5\u13F8-\u13FD\u1401-\u166C\u166F-\u167F\u1681-\u169A\u16A0-\u16EA\u16F1-\u16F8\u1700-\u170C\u170E-\u1711\u1720-\u1731\u1740-\u1751\u1760-\u176C\u176E-\u1770\u1780-\u17B3\u17D7\u17DC\u1820-\u1877\u1880-\u1884\u1887-\u18A8\u18AA\u18B0-\u18F5\u1900-\u191E\u1950-\u196D\u1970-\u1974\u1980-\u19AB\u19B0-\u19C9\u1A00-\u1A16\u1A20-\u1A54\u1AA7\u1B05-\u1B33\u1B45-\u1B4B\u1B83-\u1BA0\u1BAE\u1BAF\u1BBA-\u1BE5\u1C00-\u1C23\u1C4D-\u1C4F\u1C5A-\u1C7D\u1C80-\u1C88\u1CE9-\u1CEC\u1CEE-\u1CF1\u1CF5\u1CF6\u1D00-\u1DBF\u1E00-\u1F15\u1F18-\u1F1D\u1F20-\u1F45\u1F48-\u1F4D\u1F50-\u1F57\u1F59\u1F5B\u1F5D\u1F5F-\u1F7D\u1F80-\u1FB4\u1FB6-\u1FBC\u1FBE\u1FC2-\u1FC4\u1FC6-\u1FCC\u1FD0-\u1FD3\u1FD6-\u1FDB\u1FE0-\u1FEC\u1FF2-\u1FF4\u1FF6-\u1FFC\u2071\u207F\u2090-\u209C\u2102\u2107\u210A-\u2113\u2115\u2119-\u211D\u2124\u2126\u2128\u212A-\u212D\u212F-\u2139\u213C-\u213F\u2145-\u2149\u214E\u2183\u2184\u2C00-\u2C2E\u2C30-\u2C5E\u2C60-\u2CE4\u2CEB-\u2CEE\u2CF2\u2CF3\u2D00-\u2D25\u2D27\u2D2D\u2D30-\u2D67\u2D6F\u2D80-\u2D96\u2DA0-\u2DA6\u2DA8-\u2DAE\u2DB0-\u2DB6\u2DB8-\u2DBE\u2DC0-\u2DC6\u2DC8-\u2DCE\u2DD0-\u2DD6\u2DD8-\u2DDE\u2E2F\u3005\u3006\u3031-\u3035\u303B\u303C\u3041-\u3096\u309D-\u309F\u30A1-\u30FA\u30FC-\u30FF\u3105-\u312E\u3131-\u318E\u31A0-\u31BA\u31F0-\u31FF\u3400-\u4DB5\u4E00-\u9FEA\uA000-\uA48C\uA4D0-\uA4FD\uA500-\uA60C\uA610-\uA61F\uA62A\uA62B\uA640-\uA66E\uA67F-\uA69D\uA6A0-\uA6E5\uA717-\uA71F\uA722-\uA788\uA78B-\uA7AE\uA7B0-\uA7B7\uA7F7-\uA801\uA803-\uA805\uA807-\uA80A\uA80C-\uA822\uA840-\uA873\uA882-\uA8B3\uA8F2-\uA8F7\uA8FB\uA8FD\uA90A-\uA925\uA930-\uA946\uA960-\uA97C\uA984-\uA9B2\uA9CF\uA9E0-\uA9E4\uA9E6-\uA9EF\uA9FA-\uA9FE\uAA00-\uAA28\uAA40-\uAA42\uAA44-\uAA4B\uAA60-\uAA76\uAA7A\uAA7E-\uAAAF\uAAB1\uAAB5\uAAB6\uAAB9-\uAABD\uAAC0\uAAC2\uAADB-\uAADD\uAAE0-\uAAEA\uAAF2-\uAAF4\uAB01-\uAB06\uAB09-\uAB0E\uAB11-\uAB16\uAB20-\uAB26\uAB28-\uAB2E\uAB30-\uAB5A\uAB5C-\uAB65\uAB70-\uABE2\uAC00-\uD7A3\uD7B0-\uD7C6\uD7CB-\uD7FB\uF900-\uFA6D\uFA70-\uFAD9\uFB00-\uFB06\uFB13-\uFB17\uFB1D\uFB1F-\uFB28\uFB2A-\uFB36\uFB38-\uFB3C\uFB3E\uFB40\uFB41\uFB43\uFB44\uFB46-\uFBB1\uFBD3-\uFD3D\uFD50-\uFD8F\uFD92-\uFDC7\uFDF0-\uFDFB\uFE70-\uFE74\uFE76-\uFEFC\uFF21-\uFF3A\uFF41-\uFF5A\uFF66-\uFFBE\uFFC2-\uFFC7\uFFCA-\uFFCF\uFFD2-\uFFD7\uFFDA-\uFFDC]|\uD800[\uDC00-\uDC0B\uDC0D-\uDC26\uDC28-\uDC3A\uDC3C\uDC3D\uDC3F-\uDC4D\uDC50-\uDC5D\uDC80-\uDCFA\uDE80-\uDE9C\uDEA0-\uDED0\uDF00-\uDF1F\uDF2D-\uDF40\uDF42-\uDF49\uDF50-\uDF75\uDF80-\uDF9D\uDFA0-\uDFC3\uDFC8-\uDFCF]|\uD801[\uDC00-\uDC9D\uDCB0-\uDCD3\uDCD8-\uDCFB\uDD00-\uDD27\uDD30-\uDD63\uDE00-\uDF36\uDF40-\uDF55\uDF60-\uDF67]|\uD802[\uDC00-\uDC05\uDC08\uDC0A-\uDC35\uDC37\uDC38\uDC3C\uDC3F-\uDC55\uDC60-\uDC76\uDC80-\uDC9E\uDCE0-\uDCF2\uDCF4\uDCF5\uDD00-\uDD15\uDD20-\uDD39\uDD80-\uDDB7\uDDBE\uDDBF\uDE00\uDE10-\uDE13\uDE15-\uDE17\uDE19-\uDE33\uDE60-\uDE7C\uDE80-\uDE9C\uDEC0-\uDEC7\uDEC9-\uDEE4\uDF00-\uDF35\uDF40-\uDF55\uDF60-\uDF72\uDF80-\uDF91]|\uD803[\uDC00-\uDC48\uDC80-\uDCB2\uDCC0-\uDCF2]|\uD804[\uDC03-\uDC37\uDC83-\uDCAF\uDCD0-\uDCE8\uDD03-\uDD26\uDD50-\uDD72\uDD76\uDD83-\uDDB2\uDDC1-\uDDC4\uDDDA\uDDDC\uDE00-\uDE11\uDE13-\uDE2B\uDE80-\uDE86\uDE88\uDE8A-\uDE8D\uDE8F-\uDE9D\uDE9F-\uDEA8\uDEB0-\uDEDE\uDF05-\uDF0C\uDF0F\uDF10\uDF13-\uDF28\uDF2A-\uDF30\uDF32\uDF33\uDF35-\uDF39\uDF3D\uDF50\uDF5D-\uDF61]|\uD805[\uDC00-\uDC34\uDC47-\uDC4A\uDC80-\uDCAF\uDCC4\uDCC5\uDCC7\uDD80-\uDDAE\uDDD8-\uDDDB\uDE00-\uDE2F\uDE44\uDE80-\uDEAA\uDF00-\uDF19]|\uD806[\uDCA0-\uDCDF\uDCFF\uDE00\uDE0B-\uDE32\uDE3A\uDE50\uDE5C-\uDE83\uDE86-\uDE89\uDEC0-\uDEF8]|\uD807[\uDC00-\uDC08\uDC0A-\uDC2E\uDC40\uDC72-\uDC8F\uDD00-\uDD06\uDD08\uDD09\uDD0B-\uDD30\uDD46]|\uD808[\uDC00-\uDF99]|\uD809[\uDC80-\uDD43]|[\uD80C\uD81C-\uD820\uD840-\uD868\uD86A-\uD86C\uD86F-\uD872\uD874-\uD879][\uDC00-\uDFFF]|\uD80D[\uDC00-\uDC2E]|\uD811[\uDC00-\uDE46]|\uD81A[\uDC00-\uDE38\uDE40-\uDE5E\uDED0-\uDEED\uDF00-\uDF2F\uDF40-\uDF43\uDF63-\uDF77\uDF7D-\uDF8F]|\uD81B[\uDF00-\uDF44\uDF50\uDF93-\uDF9F\uDFE0\uDFE1]|\uD821[\uDC00-\uDFEC]|\uD822[\uDC00-\uDEF2]|\uD82C[\uDC00-\uDD1E\uDD70-\uDEFB]|\uD82F[\uDC00-\uDC6A\uDC70-\uDC7C\uDC80-\uDC88\uDC90-\uDC99]|\uD835[\uDC00-\uDC54\uDC56-\uDC9C\uDC9E\uDC9F\uDCA2\uDCA5\uDCA6\uDCA9-\uDCAC\uDCAE-\uDCB9\uDCBB\uDCBD-\uDCC3\uDCC5-\uDD05\uDD07-\uDD0A\uDD0D-\uDD14\uDD16-\uDD1C\uDD1E-\uDD39\uDD3B-\uDD3E\uDD40-\uDD44\uDD46\uDD4A-\uDD50\uDD52-\uDEA5\uDEA8-\uDEC0\uDEC2-\uDEDA\uDEDC-\uDEFA\uDEFC-\uDF14\uDF16-\uDF34\uDF36-\uDF4E\uDF50-\uDF6E\uDF70-\uDF88\uDF8A-\uDFA8\uDFAA-\uDFC2\uDFC4-\uDFCB]|\uD83A[\uDC00-\uDCC4\uDD00-\uDD43]|\uD83B[\uDE00-\uDE03\uDE05-\uDE1F\uDE21\uDE22\uDE24\uDE27\uDE29-\uDE32\uDE34-\uDE37\uDE39\uDE3B\uDE42\uDE47\uDE49\uDE4B\uDE4D-\uDE4F\uDE51\uDE52\uDE54\uDE57\uDE59\uDE5B\uDE5D\uDE5F\uDE61\uDE62\uDE64\uDE67-\uDE6A\uDE6C-\uDE72\uDE74-\uDE77\uDE79-\uDE7C\uDE7E\uDE80-\uDE89\uDE8B-\uDE9B\uDEA1-\uDEA3\uDEA5-\uDEA9\uDEAB-\uDEBB]|\uD869[\uDC00-\uDED6\uDF00-\uDFFF]|\uD86D[\uDC00-\uDF34\uDF40-\uDFFF]|\uD86E[\uDC00-\uDC1D\uDC20-\uDFFF]|\uD873[\uDC00-\uDEA1\uDEB0-\uDFFF]|\uD87A[\uDC00-\uDFE0]|\uD87E[\uDC00-\uDE1D])\S*/g;

This comment has been minimized.

@gkalpak

gkalpak Mar 15, 2018

Member

This will match strings like one,two,three, true|false or foo-vs-bar as a single "word" (which is different than what happened before). Just pointing it out to make sure we are OK with it and consider whether it qualifies as a breaking change.
(cc @vicb)

This comment has been minimized.

@skryvets

skryvets Mar 16, 2018

Contributor

@vicb, @gkalpak
Do you want me to find the solution to support existing cases as well(one,two,three true|false or foo-vs-bar) or just adjust with nits above and push the changes?

Thank you!

This comment has been minimized.

@vicb

vicb Mar 16, 2018

Contributor

Would /(?:\p{L})+/u fix this ? (using the concise form here to stay... well concise)

This comment has been minimized.

@vicb

vicb Mar 16, 2018

Contributor

I guess it would but it would break "it's"...

I would vote to say that the current state is good enough. Maybe add a test show how one,two,three, true|false or foo-vs-bar are handled ?

What do you guys think ?

This comment has been minimized.

@gkalpak

gkalpak Mar 16, 2018

Member

Yeah, /\p{L}+/u would fix some cases, but break others. I.e. I don't think there is any one "correct" solution. It depends on your usecase. I think the current implementation is fine, but I would document what the behavior is (i.e. that as far as this pipe is concerned, a "word" is an string starting with a letter and followed by any number of non-whitespace characters).

It remains to be determined, whether this qualifies as a breaking change. I am not sure what the "policy" is 😁

This comment has been minimized.

@vicb

vicb Mar 16, 2018

Contributor

@gkalpak sounds good to me.

Could we agree on:

  • We keep the current implementation for the PR,
  • We add explicit API documentation on the pipe,
  • We add explicit tests,
  • We merge this only in master (NOT in patch). There is a slight change in behavior. While I don't think this will break anything, it's safer to target master only.

@skryvets skryvets force-pushed the skryvets:fix-titlecase-pipe branch 2 times, most recently from f664b03 to 4168462 Mar 17, 2018

@skryvets

This comment has been minimized.

Contributor

skryvets commented Mar 17, 2018

@vicb, @gkalpak

I updated this PR with the following changes:

  • Change Unicode Letter to Unicode word variable name and adjust the comment properly
  • Moved transpiled regex at a file level
  • Add explicit API documentation on the pipe (Please see the screenshot)
  • Add explicit tests (for cases like one,two,three, true|false or foo-vs-bar)
@gkalpak

gkalpak requested changes Mar 19, 2018 edited

Almost LGTM 😁
Thx, @sergeome 👍

expect(pipe.transform('poniedziałek')).toEqual('Poniedziałek');
expect(pipe.transform('éric')).toEqual('Éric');
});

This comment has been minimized.

@gkalpak

gkalpak Mar 19, 2018

Member

Could you also add a test for other whitespace characters (e.g. \t, \r, \n`)?

<p>{{'one,two,three' | titlecase}} - output is expected to be 'One,two,three'</p>
<p>{{'true|false' | titlecase}} - output is expected to be 'True|false'</p>
<p>{{'foo-vs-bar' | titlecase}} - output is expected to be 'Foo-vs-bar'</p>
</div>`

This comment has been minimized.

@gkalpak

gkalpak Mar 19, 2018

Member

It would be awesome if you could write an e2e test for this :)
This will ensure that the next time someone modifies the TitleCasePipe, they won't forget to also update the example and it will remain correct 😁

You can add the e2e tests in packages/examples/common/pipes/ts/e2e_test/pipe_spec.ts.

/**
* Transforms text to titlecase.
*
* The pipe uses space as a separator for each word.

This comment has been minimized.

@gkalpak

gkalpak Mar 19, 2018

Member

Since our docs tooling will extract the first paragraph ("Transforms text to titlecase.") as a "short description", the rest of the text will become the main/"long" description of the pipe an appear at the relevant section. Currently, this shows as:

Description

The pipe uses space as a separator for each word.

Which is a bit out of context. Could expand it a bit (e.g. explaining that it splits the text up into "words" and capitalizes each word or something)?

This comment has been minimized.

@gkalpak

gkalpak Mar 19, 2018

Member

Or maybe you can just remove the empty line, making both lines appear in "short description".

@skryvets skryvets force-pushed the skryvets:fix-titlecase-pipe branch from 4168462 to c3c151a Mar 21, 2018

@skryvets

This comment has been minimized.

Contributor

skryvets commented Mar 21, 2018

As discussed with @gkalpak, the PR was updated with the following changes:

  • Add unit tests to cover whitespace characters like \t, \r, \n
  • Add e2e tests for the example
  • Refactor existing e2e tests for the pipe to move them into their own describe block
  • Change title pipe description to make it appear in "short description" section
@mary-poppins

This comment has been minimized.

mary-poppins commented Mar 21, 2018

/**
* Transforms text to titlecase.
* The pipe splits up a text into words, capitalizes the first letter of each word and transforms
* rest of the word into lowercase. In this case space is used as a separator (including other

This comment has been minimized.

@gkalpak

gkalpak Mar 21, 2018

Member

rest --> the rest (I think)
Also, I think space usually refers to , not other whitespace characters, so I would change the last sentence to something like:

In this case, whitespace characters (such as , \t, \n, etc) are used as word separators.

/**
* Transforms text to titlecase.
* The pipe splits up a text into words, capitalizes the first letter of each word and transforms
* the rest of the word into lowercase. In this case, whitespace characters (such as , \t, \n, etc)

This comment has been minimized.

@gkalpak

gkalpak Mar 21, 2018

Member

You need to enclose the character in backticks to make them more clear.
, \t, \n --> ` `, `\t`, `\n`

@skryvets skryvets force-pushed the skryvets:fix-titlecase-pipe branch from 09502ee to 11ef9e6 Mar 21, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Mar 21, 2018

/**
* Transforms text to titlecase.
* The pipe splits up a text into words, capitalizes the first letter of each word and transforms
* the rest of the word into lowercase. In this case, whitespace characters (such as ` `, `\t`,
* `\n`, etc)

This comment has been minimized.

@skryvets

skryvets Mar 21, 2018

Contributor

@gkalpak, updated.

This comment has been minimized.

@gkalpak

gkalpak Mar 21, 2018

Member

Thx. Unfortunately, it turns out ` ` doesn't render well (or even at all) 😇
Maybe change it to "space" (i.e. ``...(such as space, \t, `\n`, etc.)...`)?

(Sorry for the back-and-forth. We're getting there 😁)

This comment has been minimized.

@skryvets

skryvets Mar 22, 2018

Contributor

@gkalpak, updated.

@skryvets skryvets force-pushed the skryvets:fix-titlecase-pipe branch from c848a97 to ab52598 Mar 22, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Mar 22, 2018

@skryvets skryvets force-pushed the skryvets:fix-titlecase-pipe branch from ab52598 to c926782 Mar 22, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Mar 22, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Mar 22, 2018

* The pipe splits up a text into words, capitalizes the first letter of each word and transforms
* the rest of the word into lowercase. In this case, whitespace characters (such as "space", "\t",
* "\n", etc) are used as word separators.
* ## Example

This comment has been minimized.

@gkalpak

gkalpak Mar 22, 2018

Member

Leave an empty line above ## Example so that it is not part of the short description.

This comment has been minimized.

@skryvets

skryvets Mar 22, 2018

Contributor

@gkalpak, If I leave an empty line above ## Example there will be empty description.

screenshot1

Another option is to leave short description as is and add "long description", e.g.:

screenshot2

Did you mean second option? Just to make sure that we're on the same page 😃

This comment has been minimized.

@gkalpak

gkalpak Mar 22, 2018

Member

Oh, I didn't realize there will be an empty description 😞
The second option looks good 👍

@skryvets skryvets force-pushed the skryvets:fix-titlecase-pipe branch from c926782 to 740cdeb Mar 22, 2018

@skryvets

This comment has been minimized.

Contributor

skryvets commented Mar 22, 2018

@gkalpak, I updated the code in accordance with the second picture.

@mary-poppins

This comment has been minimized.

mary-poppins commented Mar 22, 2018

@gkalpak

This comment has been minimized.

Member

gkalpak commented Mar 23, 2018

Awesome! Thx @sergeome 👍
@vicb, ready for your review 😃

@vicb

vicb approved these changes Mar 23, 2018

@vicb

This comment has been minimized.

Contributor

vicb commented Mar 23, 2018

@sergeome Thank you very much for your work on this ! (and sorry for the delays in reviewing).

@vicb

This comment has been minimized.

@matsko matsko closed this in 7966744 Mar 23, 2018

@gkalpak gkalpak referenced this pull request Mar 23, 2018

Closed

fix(common): inconsistency on TitleCasePipe #22322

3 of 3 tasks complete

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

@Pablosky12

This comment has been minimized.

Pablosky12 commented Apr 29, 2018

This is still having problems when the letter is an Ñ. It adds titleCase to that letter and the next character

@gkalpak

This comment has been minimized.

Member

gkalpak commented Apr 30, 2018

@Pablosky12, you are probably using a version that does not contain the fix. (Note it has been fixed on 6.x only, which is still in RC.)
It works on the latest rc.

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