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: Migrate block viewer to React #2980

Merged
merged 13 commits into from
Aug 21, 2020
Merged

Conversation

onprem
Copy link
Member

@onprem onprem commented Aug 4, 2020

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

Changes

  • Migrated the Block Viewer to React

Verification

Added new unit tests.

Screenshots

image

image

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Copy link
Member

@GiedriusS GiedriusS 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 so far! Very nice start! 🎉 Everything so far is just like we've talked 👏 Also, the logic of calculating the widths/etc. looks sound. Can't comment on the React code patterns, though.

Some things that are missing from the top of my head (I might be wrong 😄 ):

  • Different colors for different combos of compaction levels and resolutions
  • Darker shades for overlapping blocks
  • The size of the block in the pop-up on the right

Makefile Show resolved Hide resolved
pkg/ui/react-app/src/thanos/pages/blocks/Blocks.tsx Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@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.

Great work so far Prem! I'll try to fire this up today :)

pkg/ui/react-app/src/thanos/Navbar.tsx Show resolved Hide resolved
pkg/ui/react-app/src/thanos/pages/blocks/TimeRange.tsx Outdated Show resolved Hide resolved
pkg/ui/react-app/src/thanos/pages/blocks/blocks.module.css Outdated Show resolved Hide resolved
@onprem
Copy link
Member Author

onprem commented Aug 6, 2020

@GiedriusS

Different colors for different combos of compaction levels and resolutions

We now have different colors for diff resolutions, but I still need to figure out what we need to do about different compaction levels. One idea is to have a darker shade of color for a higher compaction level and a lighter shade of same color for a lower compaction level.

Darker shades for overlapping blocks

It slipped my mind, thanks for reminding me. I have some ideas on how to implement this.

The size of the block in the pop-up on the right

Ah yes. But we need to first have this data in the API, maybe we can tackle this in a separate PR, WDYT?

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
@bwplotka
Copy link
Member

Important part that was discussed offline: How overlap with 10 blocks looks like e.g?

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
@onprem onprem marked this pull request as ready for review August 15, 2020 09:22
@onprem
Copy link
Member Author

onprem commented Aug 15, 2020

@bwplotka

Important part that was discussed offline: How overlap with 10 blocks looks like e.g?

For now, if some blocks are overlapping, the overlapping part will turn darker in shade compared to the rest of the blocks in the same row. The shade doesn't depend on the number of overlapping blocks though, so if there are 2 blocks overlapping or 10, the effect will be the same.

In the future, we can move the overlapping blocks in a new row. This will make more sense if someday we start supporting vertical compaction.

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Prem Kumar <prmsrswt@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.

Overall this looks great to me :)))) Let's merge this and iterate more

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Awesome work, Prem! 😻

@GiedriusS GiedriusS merged commit 5db8135 into thanos-io:master Aug 21, 2020
@onprem onprem deleted the react-block-viewer branch August 21, 2020 09:12
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.

4 participants