-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Monitoring] Capitalize the license type #20683
Conversation
💚 Build Succeeded |
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.
Looks good, but I think we could push this fix deeper for an even better fix.
@@ -54,12 +54,14 @@ export class LicenseViewController { | |||
expiryDate = formatDateTimeLocal(license.expiry_date); | |||
} | |||
|
|||
const capitalizedType = license.type[0].toUpperCase() + license.type.slice(1); |
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.
This does fix the issue, but I think that this is the wrong place to fix it. We have two examples of this component being used and it's already been used differently between the two.
Instead of fixing it at the caller level, I think we should just do the case management of the license type inside of the component, which eliminates the need to think about it and simplifies caller code.
Line 20 in d42fef6
const { isExpired, status, type, expiryDate } = this.props; |
If you modify type
there to have this behavior (you will also want to explicitly lowercase the type.slice(1)
portion for parity; you should probably also use type.substr(1)
like the other code), then this code change can go away and the code that you're effectively copying can also be removed:
Line 13 in 32505af
const typeTitleCase = type.charAt(0).toUpperCase() + type.substr(1).toLowerCase(); |
You may also be interested in Lodash's startCase
function.
This reverts commit 6a05bb1.
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! Thanks for dealing with my nitpick.
Anything for you @pickypg ❤️ |
💔 Build Failed |
jenkins, test this |
💚 Build Succeeded |
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
* Capitalize the license type in monitoring * Revert "Capitalize the license type in monitoring" This reverts commit 38ad3a5. * Pass in the type capitalized * Revert "Pass in the type capitalized" This reverts commit 6a05bb1. * Capitalize type in license status directly so consumers do not need to worry about it
* Capitalize the license type in monitoring * Revert "Capitalize the license type in monitoring" This reverts commit 38ad3a5. * Pass in the type capitalized * Revert "Pass in the type capitalized" This reverts commit 6a05bb1. * Capitalize type in license status directly so consumers do not need to worry about it
Backport: 6.x: a5f2725 |
* Capitalize the license type in monitoring * Revert "Capitalize the license type in monitoring" This reverts commit 38ad3a5. * Pass in the type capitalized * Revert "Pass in the type capitalized" This reverts commit 6a05bb1. * Capitalize type in license status directly so consumers do not need to worry about it
Fixes #20276
This PR gives parity between the license UI in monitoring and the license UI in management by also capitalizing the license type