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

feat(core): creation of the useInteractiveTable composable #7772

Closed
wants to merge 2 commits into from

Conversation

MathieuRA
Copy link
Member

Description

Short explanation of this PR (feel free to re-use commit message)

Checklist

  • Commit
    • Title follows commit conventions
    • Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
    • If bug fix, add Introduced by
  • Changelog
    • If visible by XOA users, add changelog entry
    • Update "Packages to release" in CHANGELOG.unreleased.md
  • PR
    • If UI changes, add screenshots
    • If not finished or not tested, open as Draft

@MathieuRA MathieuRA self-assigned this Jun 24, 2024
@MathieuRA MathieuRA requested a review from OlivierFL June 25, 2024 13:49
@@ -80,7 +80,7 @@ const currentInteraction = computed(() =>
interactions.value.find(interaction => router.currentRoute.value.query[columnName] === interaction.id)
)

const columnName = `${tableName}__${props.id}`
const columnName = stringifyColumnName(tableName, props.id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

To keep prop reactivity, this should be a computed:

Suggested change
const columnName = stringifyColumnName(tableName, props.id)
const columnName = computed(() => stringifyColumnName(tableName, props.id))

Comment on lines +70 to +71
{ id: 'sort-asc', icon: faArrowDown, label: t('core.sort.ascending') },
{ id: 'sort-desc', icon: faArrowUp, label: t('core.sort.descending') },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't forget to remove the "coming soon" tooltip inside the template when the interaction is not disabled.

Comment on lines +6 to +14
if (typeof prev === 'string' && typeof next === 'string') {
return prev.localeCompare(next)
} else if (typeof prev === 'number' && typeof next === 'number') {
return prev - next
} else if (prev instanceof Date && next instanceof Date) {
return prev.getTime() - next.getTime()
} else {
return String(prev).localeCompare(String(next))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It comes down to personal preference and coding style, but what do you think of using this suggestion to improve readability?

Suggested change
if (typeof prev === 'string' && typeof next === 'string') {
return prev.localeCompare(next)
} else if (typeof prev === 'number' && typeof next === 'number') {
return prev - next
} else if (prev instanceof Date && next instanceof Date) {
return prev.getTime() - next.getTime()
} else {
return String(prev).localeCompare(String(next))
}
if (typeof prev === 'string' && typeof next === 'string') {
return prev.localeCompare(next)
}
if (typeof prev === 'number' && typeof next === 'number') {
return prev - next
}
if (prev instanceof Date && next instanceof Date) {
return prev.getTime() - next.getTime()
}
return String(prev).localeCompare(String(next))

@OlivierFL OlivierFL requested a review from ByScripts July 1, 2024 12:27
@MathieuRA MathieuRA closed this Sep 3, 2024
@MathieuRA
Copy link
Member Author

Closed in favor of #7936

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants