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.

Sign up for GitHub

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: [Table] #295 data reactivity issue #296

Merged
merged 5 commits into from
Jan 18, 2024
Merged

fix: [Table] #295 data reactivity issue #296

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3c9b760
fix: #295 data reactivity issue
mattgoud Jan 17, 2024
bca588c
fix: [table] reactivity issue - use watch for props.items
mattgoud Jan 18, 2024
82fb082
fix: typo PuikTableScrollbarPosition
mattgoud Jan 18, 2024
3d6dd10
test: [gb-checks] select tests fail (create an other issue for this)
mattgoud Jan 18, 2024
11bbe85
test: [table] data reactivity issue
mattgoud Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/components/table/src/table.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@
</template>

<script setup lang="ts">
import { computed, ref, watch } from 'vue'
import { computed, ref, toRef, watch } from 'vue'
import { useLocale } from '@puik/hooks'
import PuikCheckbox from '../../checkbox/src/checkbox.vue'
import PuikButton from '../../button/src/button.vue'
Expand Down Expand Up @@ -285,7 +285,7 @@ const ScrollBarPosition = ref<string>('left')
const lastScrollLeft = ref(0)
const sortOrder = ref([])
const sortIcon = ref({})
const data = ref([...props.items])
const data = toRef(props, 'items')
Copy link

Choose a reason for hiding this comment

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

this is a readonly var so it should be better to have :

const data = toRef(() => props.items);

Copy link

Choose a reason for hiding this comment

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

and I just realised why there was a copy of the data here. The sort !
So as you're mutating the value, you made a copy of it beforehand.
That could be costly, depending on the data size.
It could be worth having an option for that: allowsToMutate or not.

Copy link

Choose a reason for hiding this comment

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

there is a typo:
PuikTableScrollBarPosistion
should be
PuikTableScrollBarPosition
table.ts#L16

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes that's what I did but it generates errors for me, the function expects arguments however it seems to respect the signature as in the example in the doc:

// creates a readonly ref that calls the getter on .value access
toRef(() => props.foo)

I will use watch() as an alternative

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and I just realised why there was a copy of the data here. The sort ! So as you're mutating the value, you made a copy of it beforehand. That could be costly, depending on the data size. It could be worth having an option for that: allowsToMutate or not.

yes, it can be expensive, otherwise there is a sortFromServer prop which prevents sorting on the client side. Only a SortOptions type payload is sent (which can be useful for building a server-side query instead)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes that's what I did but it generates errors for me, the function expects arguments however it seems to respect the signature as in the example in the doc:

// creates a readonly ref that calls the getter on .value access
toRef(() => props.foo)

I will use watch() as an alternative

ha! it must be because the project was still in 4.2.47...

const currentSortCol = ref('')

const resetSortIcons = () => {
Expand Down
Loading