Fixed issue where sorting icon would not show up in custom table headers#4300
Fixed issue where sorting icon would not show up in custom table headers#4300unaipme wants to merge 1 commit into
Conversation
✅ Deploy Preview for buefyv3 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Fixes BTable header rendering so the sort indicator icon is not suppressed when a column uses a custom #header slot (issue #2850).
Changes:
- Moves sort icon rendering out of the default header-label branch so it can render alongside custom header slot content.
- Preserves multi-sort icon/cancel UI by rendering it as a sibling to the header slot/label.
| <template | ||
| v-if="sortMultiple && | ||
| sortMultipleDataComputed && | ||
| sortMultipleDataComputed.length > 0 && | ||
| sortMultipleDataComputed.filter(i => | ||
| i.field === column.field).length > 0" | ||
| > | ||
| <span class="multi-sort-icons"> | ||
| <b-icon | ||
| :icon="sortIcon" | ||
| :pack="iconPack" | ||
| both | ||
| :size="sortIconSize" | ||
| :class="{ | ||
| 'is-desc': sortMultipleDataComputed | ||
| .filter(i => i.field === column.field)[0] | ||
| .order === 'desc'}" | ||
| /> |
There was a problem hiding this comment.
The multi-sort header rendering repeatedly calls sortMultipleDataComputed.filter(i => i.field === column.field) in both the v-if condition and the icon class binding. Since this block now runs even when column.$slots.header is used, this adds avoidable per-render allocations for each column. Consider replacing the filter(...).length > 0 check with some(...) and using a single find(...) result (or a computed lookup map keyed by field) for both the direction and index to avoid multiple scans.
There was a problem hiding this comment.
@copilot apply the requested change here
| </template> | ||
| <template v-else> | ||
| {{ column.label }} | ||
| <template | ||
| v-if="sortMultiple && | ||
| sortMultipleDataComputed && | ||
| sortMultipleDataComputed.length > 0 && | ||
| sortMultipleDataComputed.filter(i => | ||
| i.field === column.field).length > 0" | ||
| > | ||
| <span class="multi-sort-icons"> | ||
| <b-icon | ||
| :icon="sortIcon" | ||
| :pack="iconPack" | ||
| both | ||
| :size="sortIconSize" | ||
| :class="{ | ||
| 'is-desc': sortMultipleDataComputed | ||
| .filter(i => i.field === column.field)[0] | ||
| .order === 'desc'}" | ||
| /> | ||
| {{ findIndexOfSortData(column) }} | ||
| <button | ||
| class="delete is-small multi-sort-cancel-icon" | ||
| type="button" | ||
| @click.stop="removeSortingPriority(column)" | ||
| /> | ||
| </span> | ||
| </template> | ||
|
|
||
| <b-icon | ||
| v-else | ||
| :icon="sortIcon" | ||
| :pack="iconPack" | ||
| both | ||
| :size="sortIconSize" | ||
| class="sort-icon" | ||
| :class="{ | ||
| 'is-desc': !isAsc, | ||
| 'is-invisible': currentSortColumn !== column | ||
| }" | ||
| /> | ||
| </template> | ||
| <template |
There was a problem hiding this comment.
This change fixes UI behavior (sort icons now render even with a custom #header slot), but there’s no unit test covering it. Please add a Table test that mounts a sortable column with a header slot and asserts the .sort-icon (or multi-sort icons) are present/visible when the column is the current sort, to prevent regressions of #2850.
Fixes #2850.
The issue can still be reproduced with Buefy 3.0.4 in this CodeSandbox.
Proposed fix
Just changed the arrangement of the header slot so that the sort icon logic falls outside of the slot handling.
However, it could be argued that when the user creates a custom header using the
#headerslot, it's up to them to implement the icon too. Then, as this comment mentioned in the original issue (back in 2020, what a year that was):I think this solution would be messier, as we would have to pass a callback method or something as a prop to the slot, nevermind having to update the docs! (For legal reasons, this is a joke) Obviously, feel free to reject this PR if you do agree with the quote.