-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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: load slack channels earlier #29846
Conversation
@supersetbot label 4.1 |
value: NotificationMethodOption.SlackV2, | ||
}); | ||
if ( | ||
method && |
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 line shouldn't be necessary, right?
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.
It's to help typescript assert that the value exists, rather than runtime execution. e.g., for NotificationMethodOption
as it doesn't include undefined
. I could coerce the type, but I think that sometimes looks like a shortcut, too. wdyt?
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'm going to merge, but if you suggest another way to approach this, I'll put it up in a new PR.
(cherry picked from commit 0c3aa7d)
SUMMARY
Loading slack channels earlier to ensure that the method dropdowns have the correct value when they are loaded. Instead of loading slack channels when the slack option is selected, I am now loading the options when the notification method is rendered. That way, we know if we need to use slack v1 or v2 by the time the notification method options are rendered (since we filter out one of the options, so that only one slack option displays)
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
no visual changes
TESTING INSTRUCTIONS
If the feature flag is on, but the scopes are missing, a v1 (textarea input field) should display for slack.
ADDITIONAL INFORMATION