-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Create system banner API handler #2607
Conversation
I'll review tomorrow. |
@bryk It's a bit too early for a review. It would be better for you to wait for frontend part :) |
:) Backend code looks good. For the frontend, how about we put it in an action bar-like thing on the top. Or below the blue part. I think I prefer below the blue part. Your opinions? |
@bryk I agree. |
} | ||
|
||
// GetSeverity returns one of allowed severity values based on given parameter. | ||
func GetSeverity(severity string) string { |
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'd add type that maps to a string (i.e. type BannerSeverity string
) so we can return our type here. It's easier to find out correct values later on.
Maybe also use a switch on string here as it feels like a perfect place for it.
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.
+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.
Done.
) | ||
|
||
// SettingsManager is used for user settings management. | ||
type SettingsManager interface { |
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.
SettingsManager?
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.
Done.
Codecov Report
@@ Coverage Diff @@
## master #2607 +/- ##
==========================================
- Coverage 54.18% 54.18% -0.01%
==========================================
Files 563 563
Lines 12108 12124 +16
==========================================
+ Hits 6561 6569 +8
- Misses 5289 5297 +8
Partials 258 258
Continue to review full report at Codecov.
|
Ping me when this is ready for rereview. |
@bryk PTAL |
|
||
/** | ||
* @export | ||
* @return {*} |
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.
{sting}? If possible
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.
Unfortunately not as trustAsHtml
does not have precise return type and it can be {Object|string|null}
. https://docs.angularjs.org/api/ng/service/$sce#trustAsHtml
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.
Unfortunately not possible.
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
Closes #2603.