Skip to content
This repository has been archived by the owner on Dec 30, 2022. It is now read-only.

#393 - Feature: pass in the page when emitting the page change event #394

Merged
merged 3 commits into from
Feb 15, 2018

Conversation

VinceG
Copy link
Contributor

@VinceG VinceG commented Feb 14, 2018

fixes #393

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

Thank you! Since people didn't expect anything in that callback, we can release this as a patch I think

@Haroenv Haroenv requested a review from rayrutjes February 14, 2018 18:20
@Haroenv
Copy link
Contributor

Haroenv commented Feb 14, 2018

Can you edit the test too? (toHaveBeenCalledWith)

test('it should emit a "page-change" event when page changes', () => {
const searchStore = {
page: 1,
totalPages: 20,
totalResults: 4000,
};
const Component = Vue.extend(Pagination);
const vm = new Component({
propsData: {
searchStore,
},
});
const onPageChange = jest.fn();
vm.$on('page-change', onPageChange);
vm.$mount();
expect(onPageChange).not.toHaveBeenCalled();
vm.$el
.getElementsByTagName('li')[3]
.getElementsByTagName('a')[0]
.click();
expect(onPageChange).toHaveBeenCalledTimes(1);
});

@VinceG
Copy link
Contributor Author

VinceG commented Feb 14, 2018

@Haroenv Done.

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests, I think you changed the wrong test though.

@@ -108,7 +108,7 @@ test('it should emit a "page-change" event when page changes', () => {

vm.$mount();

expect(onPageChange).not.toHaveBeenCalled();
expect(onPageChange).not.toHaveBeenCalledWith('page');
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have stayed the same, it's testing that it wasn't called yet

@@ -108,7 +108,7 @@ test('it should emit a "page-change" event when page changes', () => {

vm.$mount();

expect(onPageChange).not.toHaveBeenCalled();
expect(onPageChange).not.toHaveBeenCalledWith('page');

vm.$el
.getElementsByTagName('li')[3]
Copy link
Contributor

Choose a reason for hiding this comment

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

after these lines add:

expect(onPageChange).toHaveBeenCalledWith(page);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Haroenv How about this:

test('it should emit a "page-change" event when page changes and pass in the page variable', () => {
  const searchStore = {
    page: 1,
    totalPages: 20,
    totalResults: 4000,
  };
  const Component = Vue.extend(Pagination);
  const vm = new Component({
    propsData: {
      searchStore,
    },
  });

  const onPageChange = jest.fn();
  vm.$on('page-change', onPageChange);

  vm.$mount();

  expect(onPageChange).not.toHaveBeenCalled();

  vm.$el
    .getElementsByTagName('li')[3]
    .getElementsByTagName('a')[0]
    .click();
  expect(onPageChange).toHaveBeenCalledTimes(1);

  const page = searchStore.page;
  expect(onPageChange).toHaveBeenCalledWith(page);
  expect(page).toBe(2);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly what I meant!

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

Great! Thanks for your contribution. I’ll release this soon

@Haroenv Haroenv merged commit a349e19 into algolia:master Feb 15, 2018
Haroenv pushed a commit to algolia/instantsearch that referenced this pull request Dec 28, 2022
algolia/vue-instantsearch#394)

* algolia/vue-instantsearch#393 - Feature: pass in the page when emitting the page change event

* updated test

* updated tests
@VinceG VinceG deleted the page-change-event branch August 7, 2023 16:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: pass in the page when emitting the page change event
2 participants