Trendyol / baklava

Baklava is a design system provided by Trendyol to create a consistent UI/UX for app users.
https://baklava.design/
MIT License
1.27k stars 112 forks source link

Double @bl-change event emitted in pagination component with multiple instances. #396

Closed mehmetranas closed 1 year ago

mehmetranas commented 1 year ago

Describe the bug When multiple instances of the pagination component are used, a single page change action triggers multiple @bl-change events, instead of the expected single event. This issue may affect the functionality and accuracy of applications using the affected component.

To Reproduce Steps to reproduce the behavior:

  1. Go to https://codesandbox.io/s/vue-2-playground-forked-buxp7u?file=/src/components/Pagination.vue
  2. Click on pagination buttons
  3. See pagination change event count increase twice every time
  4. Then click remove second pagination checkbox and click pagination buttons again
  5. See pagination change event count increase one by one.

Expected behavior It should emit an event only once, regardless of the number of instances present.

Desktop (please complete the following information):

muratcorlu commented 1 year ago

Hi @mehmetranas,

Thanks for the report. In your example, when you change the page in the first pagination, you set the currentPage of the second pagination component that causes a bl-change event on the second pagination component. This is expected behavior. What was your expectation?

mehmetranas commented 1 year ago

We use two pagination components in our data tables, one at the top and one at the bottom. When a user changes one of the paginations, the other one also triggers a change event, resulting in a double network request for obtaining the table items.

Steps to Reproduce:

The expected behavior is that only the bottom pagination component should fire a change event and initiate a single network request. The top pagination component should change the value of the "currentPage" prop, but it should not trigger a change event.

muratcorlu commented 1 year ago

Since pagination component is not aware of other components in the page, you basically ask for firing bl-change event only when current page is changed internally, not by setting currentPage outside.

That looks make sense. To benchmarking the idea, I checked this with a native input and noticed that setting value property of input doesn't trigger input event.

On the other hand, I'm not sure if there is a valid use-case to fire bl-change event as a result of a change of a property. For example, currently if you set total-items with a lower value then current-page * items-per-page then current-page changes automatically to the last page. In this case if pagination will not fire an event, developer will need to check this situation. Maybe we can consider to add a property to the event data about the source of this change instead of removing it completely.

But I think we should consider fixing or improving this anyway.

muratcorlu commented 1 year ago

BTW, you could just check if currentPage is the same with selectedPage from event to prevent doing double requests:

    onChange({ detail }) {
      if (this.currentPage === detail.selectedPage) {
        return;
      }
      ... // rest of the code
mehmetranas commented 1 year ago

Thank you, that was a good trick. But actually need one more check :)

onChange({ detail }) {
      if (this.currentPage === detail.selectedPage && this.itemsPerPage === detail.itemsPerPage) {
        return;
      }
      ... // rest of the code
DamlaDemir commented 1 year ago

When I glance at the pagination component I couldn't catch the case in which currentPage changes to the last page. Where is this control @muratcorlu ?

In my opinion, to fire event when changes currentPage and itemsPerPage seems unnecessary. We can fire this event only internally.

But if this tracking is removed, the pagination component will not track currentPage and itemsPerPage changes from coming outside. While we use the pagination components, we change pages with page buttons, go to input, or navigation buttons and change itemsPerPage with pagination size select. We don't change the currentPage or itemsPerPage as automatically.

I am not sure, is there any usage or need such as automatically change for this properties in outside of pagination component ?

muratcorlu commented 1 year ago

Setting currentPage and itemsPerPage outside is a valid requirement as in the example. If you put pagination on top and below of a long table, you'll want to sync currentPage and itemsPerPage on both pagination components. If use change the page or items per page value in one, other should also automatically update.

When I check the code, I see we are setting currentPage to 1 once itemsPerPage is changed:

  private _selectHandler(event: CustomEvent) {
    this.itemsPerPage = +event?.detail[0]?.value || 100;
    this.currentPage = 1;
  }

This can cause some inconsistencies for the pagination state outside and inside of the pagination component.

mehmetranas commented 1 year ago

Upon reviewing the case again, I saw that the 'itemsPerPage' value is not reflected in the select box of the other pagination. I believe that the code is updating the value, but the select box is not being refreshed after every change in 'itemsPerPage'.

To Reproduce Steps to reproduce the behavior:

  1. Navigate to https://codesandbox.io/s/vue-2-playground-forked-sg7h5q?file=/src/components/Pagination.vue
  2. Change the 'itemsPerPage' value using the buttons.
  3. Observe that while the value of 'itemsPerPage' is updated in the code, only one of the pagination's dropdown reflects the change.
  4. The other pagination's dropdown remains unchanged, causing the two paginations to be out of sync.

In my Vue app I found a solution to the issue by adding a key that updates every time the 'itemsPerPage' value is changed. With this key in place, both pagination dropdowns now update as expected and the two paginations are in sync.

olkeoguz commented 1 year ago

Upon reviewing the case again, I saw that the 'itemsPerPage' value is not reflected in the select box of the other pagination. I believe that the code is updating the value, but the select box is not being refreshed after every change in 'itemsPerPage'.

To Reproduce Steps to reproduce the behavior:

  1. Navigate to https://codesandbox.io/s/vue-2-playground-forked-sg7h5q?file=/src/components/Pagination.vue
  2. Change the 'itemsPerPage' value using the buttons.
  3. Observe that while the value of 'itemsPerPage' is updated in the code, only one of the pagination's dropdown reflects the change.
  4. The other pagination's dropdown remains unchanged, causing the two paginations to be out of sync.

In my Vue app I found a solution to the issue by adding a key that updates every time the 'itemsPerPage' value is changed. With this key in place, both pagination dropdowns now update as expected and the two paginations are in sync.

I think what you mention here may be related to the issue of https://github.com/Trendyol/baklava/issues/317

github-actions[bot] commented 1 year ago

:tada: This issue has been resolved in version 2.0.0-beta.81 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 1 year ago

:tada: This issue has been resolved in version 2.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: