Skip to content
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

Merged
merged 8 commits into from
May 16, 2023

Conversation

ivanahuckova
Copy link
Member

@ivanahuckova ivanahuckova commented May 15, 2023

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. - #68089

Which issue(s) does this PR fix?:

Part of #68089

revert_button.mov

Special notes for your reviewer:

  • The button had to be removed from Collapse component because Collapse is button and there is css button in button conflict. Therefore I had to move it from Collapse.
  • Currently, the button only appears when you change the query. I have also considered showing button all the time but there were couple of issues:
    • Tooltip doesn't show up if button is disabled. So you end up with disabled button and you don't understand its purpose. Also, this means that nowhere in grafana we use tooltip for disabled button (I don't think we have many disabled buttons in general) and therefore we would have to customize code and create new pattern.
    • for me it can seem confusing if you have there button if you've never changed the log context query and you don't see editor.
      So I decided to only show it when query is changed. We can re-evaluate this, if someone have strong feelings about it.

@github-actions
Copy link
Contributor

Backend code coverage report for PR #68484
No changes

@github-actions
Copy link
Contributor

github-actions bot commented May 16, 2023

Frontend code coverage report for PR #68484

Plugin Main PR Difference
loki 84.67% 84.65% -.02%

@ivanahuckova ivanahuckova self-assigned this May 16, 2023
@ivanahuckova ivanahuckova added this to the 10.1.x milestone May 16, 2023
@ivanahuckova ivanahuckova changed the title Loki: Add functionality to revert to initial query Loki: Add functionality to revert to initial query in log context May 16, 2023
@ivanahuckova ivanahuckova marked this pull request as ready for review May 16, 2023 13:21
@ivanahuckova ivanahuckova requested a review from a team as a code owner May 16, 2023 13:21
@matyax
Copy link
Contributor

matyax commented May 16, 2023

This is a very nice addition!

Copy link
Contributor

@matyax matyax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Contributor

@svennergr svennergr left a 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?

@ivanahuckova
Copy link
Member Author

I played around with it and I think I'd prefer to go with the disabled button way.

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.

Fixed in:: https://github.com/grafana/grafana/pull/68484/files/50414156b6896ba392672a4c216f07c791dccfb9..b326e95a302debe470374be5b81cae1b182244ec

For disabled state, we should probably have different tooltip, so I will create a separate PR and ask docs team for help with this.

Copy link
Contributor

@svennergr svennergr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cookie Monster

@ivanahuckova ivanahuckova merged commit 1462ae9 into main May 16, 2023
7 checks passed
@ivanahuckova ivanahuckova deleted the ivana/context-revert branch May 16, 2023 16:58
ryantxu pushed a commit that referenced this pull request May 16, 2023
…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
svennergr pushed a commit that referenced this pull request Jul 12, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants