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

Beds 1037/large dashboards should not load slot viz icons and absolute values #1303

Conversation

enzo-bitfly
Copy link
Contributor

@enzo-bitfly enzo-bitfly commented Jan 29, 2025

Changes:

SlotViz:

  • Add initial state and persistance to SlotViz icons
  • Restructure component hierarchy for slot-viz section (duties toggle, group selector and slots)
  • Prevent extra slot-viz fetch that happened between changing dashboard and loading group-selector
  • Prevent horizontal scrollbar when the page content is at it's maximum width by making the horizontal paddings smaller (this was introduced when adapting for Gnosis, but it's a design error anyway, as the slot-viz horizontal padding is too small in the designs themselves)

Summary table:

  • Add initial state and persist absolute/relative values

ValidatorDashboardOverviewStore

  • Join store and composable that was placed in the same file, and adapt all usages to keep reactivity

Restructure of SlotViz section:

Screenshot 2025-01-29 at 17 39 41

- This change affects all files that use the reactive values of the store, so that they keep their reactivity
- Additionally, add `loading` state to the store
@marcel-bitfly
Copy link
Contributor

marcel-bitfly commented Jan 30, 2025

np: commit message
(feat(DashboardSlotViz): calculate and persist slotviz visible duties

  • default visible duties depend on the dashbnoard being shared and over or under 64 validators
  • loading/intermediate states are added to prevent flashes on the visible duties when changing dashboards)

if this is the commit which solves the issue plz put a reference in:

feat(...

See: BEDS-1037

also I think all these commits kind of handle the same thing (reason for the code change) and should be one commit
image

Copy link
Contributor

@marcel-bitfly marcel-bitfly left a comment

Choose a reason for hiding this comment

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

I can imagine this was a very challenging task, although it sounded very easy.
Nice how deep you digged into the architecture of the concerned components and that you took the chance to simplify things and create a cleaner data flow.

Some things grew in complexity, that happens sometimes, especially when you are unfamiliar with the code base and everything is tightly coupled.

In general we should not add more complexity, but abort refactoring (thats also one benefit of a clean commit structure, that we could take commits out, e.g. into another branch). But this time it does not hurt us to stay with your current implementation.

Good work in respect to the unforeseeable complexity 💪

frontend/components/dashboard/ValidatorSlotViz.vue Outdated Show resolved Hide resolved
frontend/composables/useValidatorDashboardGroups.ts Outdated Show resolved Hide resolved
class="dashboard-slot-viz-group-selector"
@update-selected-group-ids="(newGroupIdSelection) => selectedGroupIds = newGroupIdSelection"
/>
</div>

<div
v-if="(loadingSlotViz && !refetchingSlotViz) || (loadingOverview && !!dashboardKey)"
Copy link
Contributor

Choose a reason for hiding this comment

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

We talked already how this refactoring worked out for you, so I understand that such things happen sometimes.

I just have to point out that this is too complex and can for sure be avoided in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This brings up a good point: I was setting loading = true in the validator_overview_store too early, and it remained true even if the fetch wasn't performed by lack of dashboardKey in the case of guest dashboards. I can now remove the !!dashboardKey at least.

@enzo-bitfly enzo-bitfly force-pushed the BEDS-1037/large-dashboards-should-not-load-slot-viz-icons-and-absolute-values branch from dbe0096 to 5e92a8e Compare January 30, 2025 11:43
Copy link

cloudflare-workers-and-pages bot commented Jan 30, 2025

Deploying beaconchain with  Cloudflare Pages  Cloudflare Pages

Latest commit: e2c3d79
Status:⚡️  Build in progress...

View logs

Copy link
Contributor Author

@enzo-bitfly enzo-bitfly left a comment

Choose a reason for hiding this comment

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

Great review, thanks 👍

@@ -13,6 +13,7 @@
"DashboardChartSummary",
"DashboardChartSummaryFilter",
"DashboardGroupManagementModal",
"DashboardSlotViz",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rephrased the commit message and removed the scope

@@ -103,6 +105,7 @@ watch(
</BcTooltip>

<DashboardSlotVizDutyVisibilityToggle
:initially-hide-visible="isSharedDashboard && !!isLargeDashboard"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's ugly. I removed it and used the values directly in DashboardSlotVizDutyVisibilityToggle

class="dashboard-slot-viz-group-selector"
@update-selected-group-ids="(newGroupIdSelection) => selectedGroupIds = newGroupIdSelection"
/>
</div>

<div
v-if="(loadingSlotViz && !refetchingSlotViz) || (loadingOverview && !!dashboardKey)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This brings up a good point: I was setting loading = true in the validator_overview_store too early, and it remained true even if the fetch wasn't performed by lack of dashboardKey in the case of guest dashboards. I can now remove the !!dashboardKey at least.

const isLargeDashboard = computed(() => {
if (!validatorCount.value) return false

// This value is a product decision from Bitfly, and doesn't stem from any mechanism in the blockchains we handle.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// This value is a product decision from Bitfly, and doesn't stem from any mechanism in the blockchains we handle.
// this amount is a product decision

Copy link
Contributor

@marcel-bitfly marcel-bitfly left a comment

Choose a reason for hiding this comment

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

thx

- default visible duties depend on the dashbnoard being `shared` and over or under 64 validators
- loading/intermediate states are added to prevent flashes on the visible duties when changing dashboards

See: BEDS-1037
@enzo-bitfly enzo-bitfly force-pushed the BEDS-1037/large-dashboards-should-not-load-slot-viz-icons-and-absolute-values branch from 5e92a8e to e2c3d79 Compare January 30, 2025 15:37
@enzo-bitfly enzo-bitfly merged commit 45bccf5 into staging Jan 30, 2025
1 of 2 checks passed
@enzo-bitfly enzo-bitfly deleted the BEDS-1037/large-dashboards-should-not-load-slot-viz-icons-and-absolute-values branch January 30, 2025 15:39
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.

2 participants