Skip to content

feat: allow user to configure default string and separator for dislay… #844

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

Merged
merged 3 commits into from
May 12, 2021

Conversation

jake-bassett
Copy link
Contributor

This change allows displayStringEnum to have its default value and separator configured through args.

@jake-bassett jake-bassett requested a review from a team as a code owner May 12, 2021 18:44
@codecov
Copy link

codecov bot commented May 12, 2021

Codecov Report

Merging #844 (a49ff74) into main (936c51f) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #844      +/-   ##
==========================================
- Coverage   85.46%   85.46%   -0.01%     
==========================================
  Files         796      796              
  Lines       16299    16298       -1     
  Branches     1937     1937              
==========================================
- Hits        13930    13929       -1     
  Misses       2337     2337              
  Partials       32       32              
Impacted Files Coverage Δ
...lities/formatters/enum/display-string-enum.pipe.ts 100.00% <100.00%> (ø)
...c/utilities/formatters/enum/display-string-enum.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 936c51f...a49ff74. Read the comment docs.

@@ -5,7 +5,7 @@ import { displayStringEnum } from './display-string-enum';
name: 'htDisplayStringEnum'
})
export class DisplayStringEnumPipe implements PipeTransform {
public transform(value: string): string {
return displayStringEnum(value);
public transform(value?: string, unknown: string = '-', separator: string = ' '): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

variable naming: unknown shouldn't be used as it's a type - is that the defaulttValue or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@github-actions

This comment has been minimized.

// We only want the first word capitalized.
return `${startCased[0]}${startCased.substr(1).toLowerCase()}`;
return `${startCasedSeparated[0]}${startCasedSeparated.substr(1).toLowerCase()}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like I commented on this at the time but who knows:

Suggested change
return `${startCasedSeparated[0]}${startCasedSeparated.substr(1).toLowerCase()}`;
return capitalize(startCasedSeparated);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You did, but that doesn't work for certain use cases and I don't feel the alternative you provided is really any cleaner than this.

https://github.com/hypertrace/hypertrace-ui/pull/752/files#r609080471

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused - you said in that linked comment you agreed that startCase(lowerCase(provided)) is cleaner, but it looks like it got merged without making that change. What use case isn't supported?

Here, I was just suggesting capitalize which is a 1:1 replacement of the explicit impl on line 14, but using

return capitalize(lowerCase(provided)).replace(' ', separator);`

to replace lines 8-14 I think is even better.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@jake-bassett jake-bassett merged commit 491704c into main May 12, 2021
@jake-bassett jake-bassett deleted the display-string-enum-optional-args branch May 12, 2021 19:32
@github-actions
Copy link

Unit Test Results

    4 files  ±0  252 suites  ±0   16m 27s ⏱️ + 1m 14s
901 tests ±0  901 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
907 runs  ±0  907 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 491704c. ± Comparison against base commit 936c51f.

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.

2 participants