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

perf(VDataTable,VDataIterator)!: add item-id and fix check/uncheck all entries performance #20441

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

userquin
Copy link
Member

@userquin userquin commented Sep 8, 2024

Description

This PR adds a new item-id prop to VDataTable and VDataIterator using id when not configured (default).

I need to add the new item-id to the api and check the selectStrategy, looks like can be a custom object, check selectStrategy computed (L127) in packages/vuetify/src/components/VDataTable/composables/select.ts module in this PR (added TODO).

Pending tasks:

  • VDataIterator api docs includes item-key but missing in the props, should be renamed to item-id
  • update VDataTable and VDataIterator examples in docs
  • fix SelectItemId return type, maybe we can use any
  • use logic or instead nullish coalescing when getting the item-id

We should review also the item-value and change it to undefined, in the api docs using id.

The performance issue in #19447 is the isSelected computed, with this PR just a map lookup using the new item-id.

fixes #19447

Markup:

https://streamable.com/98scf9

10K rows VDataTable
<template>
  <v-app>
    <v-card title="Devices" flat>
      <template #text>
        <v-text-field
          v-model="search"
          label="Search"
          prepend-inner-icon="mdi-magnify"
          variant="outlined"
          hide-details
          single-line
        />
      </template>

      <v-data-table
        v-model="checkedDevices"
        :items="devices"
        :items-per-page-options="[
          {value: 5, title: '5'},
          {value: 10, title: '10'},
          {value: 25, title: '25'},
          {value: -1, title: 'All'},
        ]"
        :search="search"
        density="compact"
        item-id="id"
        item-value="text"
        items-per-page="5"
        select-strategy="all"
        show-select
        sticky
      />
    </v-card>
  </v-app>
</template>

<script>
  export default {
    name: 'AddEditInitiatives',
    data () {
      return {
        search: '',
        devices: [],
        checkedDevices: [],
      }
    },

    /* watch: {
      checkedDevices: val => {
        console.log(`Watch: ${val.join(', ')}`)
      },
    }, */
    mounted () {
      this.getDevices()
    },

    methods: {
      getDevices () {
        for (let i = 0; i < 10000; i++) {
          this.devices.push({
            id: i + 1,
            text: `Device ${i + 1}`,
            info: {
              serial: `#123456789-${i + 1}`,
              type: 'Phone',
              os: Math.random() > 0.5 ? 'Android' : 'IPhone',
              version: 10 * Math.random(),
              status: Math.random() > 0.5 ? 'Active' : 'Inactive',
            },
          })
        }
      },
    },
  }
</script>

@@ -70,6 +70,8 @@ export function getObjectValueByPath (obj: any, path?: string | null, fallback?:
return getNestedValue(obj, path.split('.'), fallback)
}

export type SelectItemId = string | undefined | ((item: any) => string)
Copy link
Member Author

@userquin userquin Sep 8, 2024

Choose a reason for hiding this comment

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

Add number to the function: string | number, we should review the type returned here, we also need the item-id prop, in any case, should it should be a primivite?.

We can keep string, the user should add it using proper conversion: id: number | string

@userquin userquin changed the base branch from master to dev September 8, 2024 15:42
@userquin userquin changed the base branch from dev to master September 8, 2024 15:43
@userquin userquin marked this pull request as draft September 10, 2024 13:17
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.

[Bug Report][3.5.11] Select All in V Data Table is freezing on selection-strategy="all"
1 participant