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

ui: Add search block UI #4444

Merged
merged 6 commits into from Aug 4, 2021
Merged

ui: Add search block UI #4444

merged 6 commits into from Aug 4, 2021

Conversation

adzshaf
Copy link
Contributor

@adzshaf adzshaf commented Jul 14, 2021

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

This PR is part of LFX Mentorship project (issue: #3112). We want to add Search Block by ID.

Verification

When we haven't search anything yet:

image

Example search block by ID:

image

Tested on local using make quickstart

@adzshaf
Copy link
Contributor Author

adzshaf commented Jul 14, 2021

Should we add query param so it is easier for user to reference it? e.g /loaded?id=<BLOCK_ID> @onprem @squat

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Love it! This is super useful for us. Would be good to support more filter conditions in the future (by external labels, compaction levels, etc)

@onprem
Copy link
Member

onprem commented Jul 20, 2021

Should we add query param so it is easier for user to reference it? e.g /loaded?id=<BLOCK_ID> @onprem @squat

Yep, that sounds like a nice idea!

Copy link
Member

@onprem onprem left a comment

Choose a reason for hiding this comment

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

Some nits and a few suggestions. Looking good!

pkg/ui/react-app/src/thanos/pages/blocks/BlockInput.tsx Outdated Show resolved Hide resolved
pkg/ui/react-app/src/thanos/pages/blocks/Blocks.tsx Outdated Show resolved Hide resolved
pkg/ui/react-app/src/thanos/pages/blocks/BlockInput.tsx Outdated Show resolved Hide resolved
pkg/ui/react-app/src/thanos/pages/blocks/helpers.ts Outdated Show resolved Hide resolved
const resultIndex = result.map((value) => value.index);

const blockResult = blocks.filter((block, index) => resultIndex.includes(index));
return blockResult;
Copy link
Member

Choose a reason for hiding this comment

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

I spoke with @adzshaf and mentioned that I think this could produce confusing experience for the user. If I provide full-length ULID that matches a block name exactly, but fuzzy search returns more than one result, then I would be very confused and asume that all of the blocks have the same name, which should not really be happening. I think it's fair to return multiple blocks if the ULID does not match any block exactly, i.e. it is not full-length or has a typo.

We can do this in a follow up of course, since this UI already provides a ton of value.

squat
squat previously approved these changes Aug 4, 2021
Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

lgtm!

Signed-off-by: Shafiya Adzhani <adz.arsym@gmail.com>
Signed-off-by: Shafiya Adzhani <adz.arsym@gmail.com>
Signed-off-by: Shafiya Adzhani <adz.arsym@gmail.com>
Signed-off-by: Shafiya Adzhani <adz.arsym@gmail.com>
Signed-off-by: Shafiya Adzhani <adz.arsym@gmail.com>
squat
squat previously approved these changes Aug 4, 2021
Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

LGTM-ing again, this force push just rebases on main

@squat
Copy link
Member

squat commented Aug 4, 2021

actually! this PR should definitely have a changelog entry! It provides a great new feature for the user

Signed-off-by: Shafiya Adzhani <adz.arsym@gmail.com>
Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

LGTM :)

@squat squat enabled auto-merge (squash) August 4, 2021 11:55
@squat squat merged commit 948abc3 into thanos-io:main Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants