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: Make label browser accessible in query builder #58525

Merged
merged 28 commits into from Nov 23, 2022

Conversation

gwdawson
Copy link
Member

@gwdawson gwdawson commented Nov 9, 2022

What is this feature?
This PR makes the Label browser accessible from both the query builder and the code mode

Why do we need this feature?
During user interviews we learned that some users switch from the query builder to the code mode just to use label browser and then go back to builder mode, which is unnecessary

Who is this feature for?
...

Which issue(s) does this PR fix?:
Fixes #57744

Special notes for your reviewer:
...

Before:

Screenshot 2022-11-21 at 15 43 47 Screenshot 2022-11-21 at 15 45 47

After:

Screenshot 2022-11-21 at 15 43 21 Screenshot 2022-11-21 at 15 45 24

@grafanabot
Copy link
Contributor

@grafanabot
Copy link
Contributor

it("doesn't render the label browser modal when closed", () => {
render(<LabelBrowserModal {...props} isOpen={false} />);
expect(screen.queryByRole('heading', { name: /label browser/i })).toBeNull();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Would also love to have a test case for onChange when we change the query. Feel free to drop it in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would be a good candidate for a follow-up task.

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! One minor adjustment and it's done.

@gwdawson gwdawson requested a review from matyax November 22, 2022 16:11
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.

One last change. Sorry I didn't see it before!

@grafanabot
Copy link
Contributor

@grafanabot
Copy link
Contributor

@gwdawson gwdawson requested a review from matyax November 23, 2022 10:39
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.

imagen

🙏

@gwdawson gwdawson requested a review from matyax November 23, 2022 15:37
@grafanabot
Copy link
Contributor

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.

GJ!

@gwdawson gwdawson merged commit a098bde into main Nov 23, 2022
@gwdawson gwdawson deleted the gareth/add-label-browser-to-builder-mode branch November 23, 2022 16:48
Copy link
Member

@ivanahuckova ivanahuckova left a comment

Choose a reason for hiding this comment

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

I left couple of feedback bellow. Also I remember that we talked about making it wider - slightly smaller than full screen, but taking advantage of the full screen size. Is there a reason you decided not to make it larger, now when we have modal and space?

@gwdawson
Copy link
Member Author

@ivanahuckova i like these suggestions! i'll make a pr for these changes.

Also I remember that we talked about making it wider - slightly smaller than full screen, but taking advantage of the full screen size. Is there a reason you decided not to make it larger

#58525 (comment) - I wasn't sure how this will effect any of the other modals that already exist. my plan was to take a look to see if theres a way to increase the size for only a specific modal, in this case the label browser. if you think i would be better to just increase the size for all of them please let me know.

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.

Loki: Make label browser accessible from both code and builder modes
6 participants