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

fix(entities-*): revamp hide toolbar logic #1790

Merged
merged 7 commits into from
Nov 20, 2024
Merged

fix(entities-*): revamp hide toolbar logic #1790

merged 7 commits into from
Nov 20, 2024

Conversation

Justineo
Copy link
Contributor

@Justineo Justineo commented Nov 18, 2024

Summary

KM-709

Preview PR for konnect-ui-apps: https://github.com/Kong/konnect-ui-apps/pull/4896

Note

I've updated this PR to a more comprehensive fix for several current issues.

Currently, logic related to table state and toolbar visibility is scattered across multiple places, leading to redundancy, inaccuracies, and visual inconsistencies. Here's a breakdown of the existing problems and their respective issues:

  1. Each entity list observes fetcherState to update a hasData flag, which determines the visibility of the create button:

    // reset `hasData` to show/hide the teleported Create button
    if (Array.isArray(state?.response?.data)) {
    hasData.value = state.response!.data.length > 0
    }

    Problem: It does not account for cache states in KTableData, causing layout shifts during revalidation.

  2. Each entity list observes fetcherState to toggle the hideTableToolbar flag and set it to true when the state is FetcherStatus.NoRecords:

    if (state.status === FetcherStatus.NoRecords) {
    hideTableToolbar.value = true
    } else {
    hideTableToolbar.value = false
    }

    Problem: Similar cache-awareness issues result in layout shifts when navigating away and returning to the list.

  3. EntityBaseTable also controls the visibility of toolbar content using the KTableData's state event:

    const showToolbar = (state: SwrvStateData): boolean => {
    return state.hasData || !!props.query
    }

    Problem: Toolbar visibility and its content can desynchronize (e.g., filter inputs hidden but the toolbar remains visible).

Refactored Logic

To address these issues, all related logic has been centralized within the EntityBaseTable, ensuring consistent behavior across all use cases. The refactored workflow includes:

  1. We no longer show/hide create button or toolbar content separate to the toolbar itself
  2. The toolbar is hidden if nothing has been added to the table (initial state)
  3. KTableData's hideToolbar is now calculated by table states and query
  4. If a filter query exists, the toolbar remains visible regardless of the table state (e.g., no records or no filter results)
  5. If the table has previously contained data (even if none is visible now due to clearing a query), the toolbar will remain visible as we are confident that we'll see unfiltered records
  6. Toolbar visibility remains configurable through the current hideToolbar prop, which now defaults to undefined so that we can offer the default behavior if it's not specified

@Justineo Justineo requested review from a team as code owners November 18, 2024 06:06
2eha0
2eha0 previously approved these changes Nov 18, 2024
Copy link
Contributor

@2eha0 2eha0 left a comment

Choose a reason for hiding this comment

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

LGTM

TT1228
TT1228 previously approved these changes Nov 18, 2024
nekolab
nekolab previously approved these changes Nov 18, 2024
Copy link
Contributor

@nekolab nekolab left a comment

Choose a reason for hiding this comment

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

LGTM

@Justineo Justineo marked this pull request as draft November 18, 2024 07:32
@Justineo Justineo changed the title fix(entities-*): hide toolbar on initial load fix(entities-*): revamp hide toolbar logic Nov 19, 2024
@Justineo Justineo dismissed stale reviews from nekolab, TT1228, and 2eha0 via 51a79dd November 19, 2024 04:05
Copy link
Contributor

@kaiarrowood kaiarrowood left a comment

Choose a reason for hiding this comment

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

This looks good to me, awesome work @Justineo

Copy link
Member

@portikM portikM left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@Justineo Justineo marked this pull request as ready for review November 20, 2024 20:17
@Justineo Justineo merged commit 9322956 into main Nov 20, 2024
23 checks passed
@Justineo Justineo deleted the fix/hide-toolbar branch November 20, 2024 20:20
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.

6 participants