-
Notifications
You must be signed in to change notification settings - Fork 2k
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
UI: Show message for when log collection is disabled #18823
UI: Show message for when log collection is disabled #18823
Conversation
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.
Hi @Sanskar531! The approach you're using here looks good to me in terms of what scenario we're responding to. I'm going to tag in @juliezzhou to get her feedback on the visual design of this warning dialog.
Thanks for the PR!
looks like there are 2 options based on https://imgur.com/a/i7XNgpY? I prefer the option that shows the alert on top of the code block. i think it is more consistent with the notification on other pages |
Hey guys, sorry for the confusion. The notification you are seeing in my test plan are from two different places. It uses the same component but it appears differently because of responsive design I am assuming. Here is the complete page: |
Tagging in @philrenaud for any thoughts |
Changelog added
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 looks good! Thanks, @Sanskar531 !
All good! Do you need me to do anything else? |
Nope! Checks all passed on this, so merging it to main now. Thanks again for your work on this, and look for it in an upcoming release! |
#16986
This is how it looks now:
https://imgur.com/a/i7XNgpY