primefaces / primevue

Next Generation Vue UI Component Library
https://primevue.org
MIT License
10.6k stars 1.23k forks source link

Datatable Slider filter: Filtering breaks after clearing filter when using slider in docs and elsewhere #4913

Closed Philistino closed 11 months ago

Philistino commented 11 months ago

Describe the bug

On the advanced filtering example in the DataTable docs. The Activity column is adjusted using a slider. This is a nice UX feature! However, it's behavior is not quite what the user would expect.

When the example is initialized, the filter model for that column is set (to 0-100). Because it is set, the filter icon on the column header is considered an active filter and it is given the class "p-column-filter-menu-button-active" to make it visually distinct from the columns without an active filter. This is a little unexpected because there isn't a user-set filter on the column.

When the user clicks the "clear" button that clears all filters, the filters on the other columns are removed and the filter on the activity column is reset to the default value of 0-100. But, it still appears active, like at the start. This is unexpected behavior: it should instead appear like the other columns, without an active filter. At this point, the slider still works.

When the user clicks the filter icon on the Activity column to open the column's filter submenu and then clicks the clear button in the submenu, the datamodel is actually cleared for that column and the filter icon appears like the icons on the other inactive columns. However, when the user goes back into to the column's filter submenu and tries to adjust the slider to reduce the upper bound, an error occurs and no filter is applied.

I think the reason for the error is when the filter is actually cleared, the value for the filtermodel on that column is set to undefined. When the user first adjusts the slider, there is no defined value for the lower bound of the slider's range in the change events (for instance [undefined, 99]), and this causes an error. No filter is actually applied until the user manually sets the lower limit.

This behavior feels pretty unintuitive and it is not ideal to show that the the filter is activated by defualt, even when it might not actually be filtering values.

This issue came up when I was trying to give the Slider element upper and lower bounds that reflected the min and max value of the data in the column.

One way to fix this would be to set the min value on the slider and then emit that value instead of undefined if no lower value is provided by the user, although that might be a breaking change.

Probably the better way to fix this would be to adjust the filter function to only consider defined filter variables. This gets a little funny because it would warp the definition of "between." For instance if we set the filter to [undefined, 2], is the number 1 between the two values? Probably not because undefined is not a number. But if you think about it from a filtering perspective, undefined means no lower bound, and therefore 1 should be included.

Error message from running on localhost. I am showing this one because it is more verbose than the one given on the docs but an error happens there too.

runtime-core.esm-bundler.js:41 [Vue warn]: Unhandled error during execution of native event handler 
  at <Slider modelValue=null onUpdate:modelValue=fn<onUpdate:modelValue> range=true  ... > 
  at <RenderFnWithContext key=1 field="activity" filterModel= {value: null, matchMode: 'between'}  ... > 
  at <BaseTransition onEnter=fn onLeave=fn<onLeave> onAfterLeave=fn<bound onOverlayAfterLeave>  ... > 
  at <Transition name="p-connected-overlay" onEnter=fn<bound onOverlayEnter> onLeave=fn<bound onOverlayLeave>  ... > 
  at <Portal> 
  at <ColumnFilter key=5 field="activity" type=undefined  ... > 
  at <HeaderCell key=0 column= {__v_isVNode: true, __v_skip: true, type: {…}, props: {…}, key: null, …} index=6  ... > 
  at <TableHeader columnGroup=null columns= (8) [{…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}] rowGroupMode=null  ... > 
  at <VirtualScroller ref="virtualScroller" items= (200) [{…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, …] columns= (8) [{…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}]  ... > 
  at <DataTable value= (200) [{…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, …] paginator=true class="p-datatable-gridlines"  ... > 
  at <CompetitorTable> 
  at <PracticeView onVnodeUnmounted=fn<onVnodeUnmounted> ref=Ref< Proxy(Object) {__v_skip: true} > > 
  at <RouterView> 
  at <AppLayout onVnodeUnmounted=fn<onVnodeUnmounted> ref=Ref< Proxy(Object) {__v_skip: true} > > 
  at <RouterView> 
  at <App>
warn2 @ runtime-core.esm-bundler.js:41
logError @ runtime-core.esm-bundler.js:216
handleError @ runtime-core.esm-bundler.js:208
callWithErrorHandling @ runtime-core.esm-bundler.js:160
callWithAsyncErrorHandling @ runtime-core.esm-bundler.js:166
invoker @ runtime-dom.esm-bundler.js:595
Show 6 more frames
Show less
runtime-core.esm-bundler.js:221 Uncaught TypeError: Cannot read properties of null (reading '0')
    at Proxy.onDragStart (slider.esm.js:126:30)
    at Proxy.onMouseDown (slider.esm.js:161:12)
    at _ctx.range.mergeProps.onMousedown._cache.<computed>._cache.<computed> (slider.esm.js:379:23)
    at callWithErrorHandling (runtime-core.esm-bundler.js:158:18)
    at callWithAsyncErrorHandling (runtime-core.esm-bundler.js:166:17)
    at HTMLSpanElement.invoker (runtime-dom.esm-bundler.js:595:5)
onDragStart @ slider.esm.js:126
onMouseDown @ slider.esm.js:161
_ctx.range.mergeProps.onMousedown._cache.<computed>._cache.<computed> @ slider.esm.js:379
callWithErrorHandling @ runtime-core.esm-bundler.js:158
callWithAsyncErrorHandling @ runtime-core.esm-bundler.js:166
invoker @ runtime-dom.esm-bundler.js:595
Show 6 more frames
Show less

Reproducer

See the docs for example behavior

PrimeVue version

3.42.0

Vue version

3.x

Language

TypeScript

Build / Runtime

Vue CLI App

Browser(s)

No response

Steps to reproduce the behavior

  1. Go to docs https://primevue.org/datatable/#advanced_filter
  2. Notice that the filter on the "Activity" column is active by default
  3. On the "Activity" column, click the filter icon to open the filter menu
  4. Click the clear button
  5. Click the same filter icon on the Activity column again and set the upper limit value using the slider and click apply
  6. Notice that the filter is not applied
  7. Open the console and see there is an error

Expected behavior

The filter to be applied after adjusting the slider

Philistino commented 11 months ago

This function would handle it with more nuance, at least for numerical values.

export function between(value, filter) {
  if (filter == null || (filter[0] == null && filter[1] == null)) {
    return true
  }

  // checks if value is null/undefined
  if (value == null) {
    return false
  }

  // handle cases if only one filter is set
  if (filter[0] == null && filter[1] != null) {
    return value <= filter[1]
  }
  if (filter[0] != null && filter[1] == null) {
    return filter[0] <= value
  }

  if (value.getTime) {
    return filter[0].getTime() <= value.getTime() && value.getTime() <= filter[1].getTime()
  }

  // happy!
  return filter[0] <= value && value <= filter[1]
}
github-actions[bot] commented 11 months ago

We're unable to replicate your issue, if you are able to create a reproducer or add details please edit this issue. This issue will be closed if no activities in 20 days.

Philistino commented 11 months ago

See video explanation here: https://streamable.com/5z1s95