-
Notifications
You must be signed in to change notification settings - Fork 11
Display string enum pipe #752
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
Conversation
@Pipe({ | ||
name: 'htDisplayStringEnum' | ||
}) | ||
export class DisplayStringEnumPipe implements PipeTransform { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use displayString?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Not sure that will work, or at least it shouldn't. The displayString
method shouldn't replace -
or _
. It should just basically String() any input but not format it... with some extra logic for null checks, arrays, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to overload display string with this behavior (for example "my-value" would become "My Value", which we don't want the main string pipe doing)
Codecov Report
@@ Coverage Diff @@
## main #752 +/- ##
=======================================
Coverage 85.34% 85.34%
=======================================
Files 786 788 +2
Lines 16115 16129 +14
Branches 2054 2055 +1
=======================================
+ Hits 13753 13765 +12
- Misses 2331 2333 +2
Partials 31 31
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, tests please (at least on the const function, the pipe is trivial enough to not matter)
This comment has been minimized.
This comment has been minimized.
// This removes dashes and underscores and gives all words initial caps | ||
const startCased = startCase(provided); | ||
|
||
// We only want the first word capitalized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's what we want, what about capitalize(lowerCase(provided))
- believe that works the same way as startCase:
_.lowerCase('--Foo-Bar--');
// => 'foo bar'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does look cleaner, but I think this one fails for camelCase and PascalCase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lodash should have ng pipes for each method. 'text' | lowercase | capitalize
Description
We keep using the same code over and over. Turning it into a const method and pipe.