-
Notifications
You must be signed in to change notification settings - Fork 12.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
Loki: Add functionality to revert to initial query in log context #68484
Conversation
Backend code coverage report for PR #68484 |
Frontend code coverage report for PR #68484
|
This is a very nice addition! |
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!
public/app/plugins/datasource/loki/components/LokiContextUi.test.tsx
Outdated
Show resolved
Hide resolved
public/app/plugins/datasource/loki/components/LokiContextUi.tsx
Outdated
Show resolved
Hide resolved
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 played around with it and I think I'd prefer to go with the disabled button way.
Basically you can use a Tooltip
on disabled buttons if you implement the button like this:
<Tooltip content="Revert to initial log context query.">
<div className={styles.iconButton}>
<Button
icon="history-alt"
variant="secondary"
disabled={isInitialQuery}
onClick={(e) => {
e.stopPropagation();
setContextFilters((contextFilters) => {
return contextFilters.map((contextFilter) => {
return {
...contextFilter,
// For revert to initial query we need to enable all labels and disable all parsed labels
enabled: !contextFilter.fromParser,
};
});
});
}}
/>
</div>
</Tooltip>
Screen.Recording.2023-05-16.at.16.47.30.mov
Potentially we can also change the tooltip message to something else when the button is disabled. WDYT?
public/app/plugins/datasource/loki/components/LokiContextUi.tsx
Outdated
Show resolved
Hide resolved
public/app/plugins/datasource/loki/components/LokiContextUi.tsx
Outdated
Show resolved
Hide resolved
Hahaha, I still prefer when button is not there cause it seems less confusing to me why it is disabled. And it appears only when you need it. But if both of you @niat22 and @svennergr think that disabled is better, then let's do it. For disabled state, we should probably have different tooltip, so I will create a separate PR and ask docs team for help with this. |
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.
…8484) * Loki: Add functionality to revert to initial query * Add tests and fix button in button * Use usereven instead of fireEvent * Shortern onClick * Add feature tracking * Use testid instead of title * Always show revert button * Remove unused imports
…8484) * Loki: Add functionality to revert to initial query * Add tests and fix button in button * Use usereven instead of fireEvent * Shortern onClick * Add feature tracking * Use testid instead of title * Always show revert button * Remove unused imports
What is this feature?
In this PR, we are introducing
Revert to initial query button
. The goal is to give users a quick way to reset query to initial query after they changed it.Why do we need this feature?
This button is going to be crucial in
preserve labels from previous context session
functionality. - #68089Which issue(s) does this PR fix?:
Part of #68089
revert_button.mov
Special notes for your reviewer:
Collapse
component becauseCollapse
isbutton
and there is cssbutton in button
conflict. Therefore I had to move it fromCollapse
.So I decided to only show it when query is changed. We can re-evaluate this, if someone have strong feelings about it.