carbon-design-system / carbon-components-svelte

Svelte implementation of the Carbon Design System
https://svelte.carbondesignsystem.com
Apache License 2.0
2.69k stars 261 forks source link

[Pagination] page error when pageSize changed #1634

Open diantongren opened 1 year ago

diantongren commented 1 year ago
image

change the page size(bigger) on the last page it will change twice.

image

here is the example

<script>
    import { Pagination } from 'carbon-components-svelte';

    let page = 1;
    let pageSize = 10;
    let pageSizes = [10, 15, 20];
    let totalItems = 35;

    $: console.log(`page: ${page}, pageSize: ${pageSize}`);
</script>

<Pagination bind:page bind:pageSize pageSizes={pageSizes} totalItems={totalItems} />

I send a request to get data when page changed, then I change pagination when I got the new data. This bug may lead to an endless cycle.

metonym commented 1 year ago

This seems similar to the issue outlined in #1491. The root cause is that the update event is set in a reactive statement where the actual desired behavior is that it's fired "once" when the UI select value changes.

Can you use the dispatched on:change event to handle pagination? That's what #1497 was designed to solve.

<Pagination
  on:change={(e) => {
    console.log("e", e.detail); // { page: number }
  }}
/>
diantongren commented 1 year ago

This seems similar to the issue outlined in #1491. The root cause is that the update event is set in a reactive statement where the actual desired behavior is that it's fired "once" when the UI select value changes.

Can you use the dispatched on:change event to handle pagination? That's what #1497 was designed to solve.

<Pagination
  on:change={(e) => {
    console.log("e", e.detail); // { page: number }
  }}
/>

thanks to reply

I use on:change, but still error when I changed the page size I changed page size from 10 to 15 image image

<Pagination pageSizes={[10, 15, 20]}
            on:change={(e) => {console.log('detail: ', e.detail)}}/>

version: 0.72.1

theetrain commented 1 year ago

Hi @diantongren, I tried reproducing your issue with this REPL: https://svelte.dev/repl/44c5b1191a5e41adb3614024f9c8a6fe?version=3.55.1

When I change the page size from 10 to 15, I see the correct value in the console. image

Feel free to fork my REPL and see if you can make an example that reproduces the issue you've reported.

diantongren commented 1 year ago

Hi @diantongren, I tried reproducing your issue with this REPL: https://svelte.dev/repl/44c5b1191a5e41adb3614024f9c8a6fe?version=3.55.1

When I change the page size from 10 to 15, I see the correct value in the console. image

Feel free to fork my REPL and see if you can make an example that reproduces the issue you've reported.

I tried it on Mac with edge, safari and arc browser, I got the right value. But I got the wrong value on Windows with chrome, edge, firefox.

theetrain commented 1 year ago

Thanks for uncovering the issue; it appears this is caused by the ordering of bind:this and on:change in Select:

https://github.com/carbon-design-system/carbon-components-svelte/blob/57b6ea68b53368240b07faff80a8a8157fae0786/src/Pagination/Pagination.svelte#L137-L147

I believe placing bind:selectedId as the first <Select> prop should resolve the issue—for both <Select>s used in Pagination.

Here is a demo illustrating the differences: https://svelte.dev/repl/d5e85d47d87b482cbaef7960c1016c1f?version=3.55.1

Docs: https://svelte.dev/docs#template-syntax-element-directives-bind-property

If you're using bind: directives together with on: directives, the order that they're defined in affects the value of the bound variable when the event handler is called.

I'm unsure why this behaviour is different on macOS and Windows. I may open a ticket in the Svelte repository and link back here if I'm able to reproduce the behaviour. It may have something to do with the ordering of props in the underlying component and how props are defined by the parent.

Please let us know if you have time to contribute a fix, otherwise I'll try getting to it.

diantongren commented 1 year ago

Thanks for uncovering the issue; it appears this is caused by the ordering of bind:this and on:change in Select:

https://github.com/carbon-design-system/carbon-components-svelte/blob/57b6ea68b53368240b07faff80a8a8157fae0786/src/Pagination/Pagination.svelte#L137-L147

I believe placing bind:selectedId as the first <Select> prop should resolve the issue—for both <Select>s used in Pagination.

Here is a demo illustrating the differences: https://svelte.dev/repl/d5e85d47d87b482cbaef7960c1016c1f?version=3.55.1

Docs: https://svelte.dev/docs#template-syntax-element-directives-bind-property

If you're using bind: directives together with on: directives, the order that they're defined in affects the value of the bound variable when the event handler is called.

I'm unsure why this behaviour is different on macOS and Windows. I may open a ticket in the Svelte repository and link back here if I'm able to reproduce the behaviour. It may have something to do with the ordering of props in the underlying component and how props are defined by the parent.

Please let us know if you have time to contribute a fix, otherwise I'll try getting to it.

I'm sorry, can't help busy.

theetrain commented 1 year ago

Note to self: a more complete solution could be leveraging the native on:change event provided by Select.

This is a reactive event loop issue that may differ depending on the browser or OS, so accessing values as directly as possible, and avoiding reactive values, may be the most precise way to dispatch changed values. Inspired by #1646.

      <Select
        id="bx--pagination-select-{id + 2}"
        class="bx--select__page-number"
        labelText="Page number, of {totalPages} pages"
        inline
        hideLabel
-       on:change="{() => {
+       on:change="{e => {
-         dispatch('change', { page });
+         dispatch('change', { page: e.target.value });
        }}"
        bind:selected="{page}"
      >