Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ajax search and maxOptions. Add initial Cypress tests #2710

Merged
merged 11 commits into from Mar 17, 2022

Conversation

caseyjhol
Copy link
Member

@caseyjhol caseyjhol commented Feb 16, 2022

  • fixes Ajax search get multiple calls #2659
  • fixes Ajax search not works #2671
  • fixes maxOptions display bug #2593
  • add some basic Cypress tests/workflow
  • support passing in a boolean more argument as an additional argument to the callback function
  • remove source.load method in favor of simply using source.data. page is now passed into source.data as the 2nd argument (just like source.search). If you want static data on init (and don't want to call your AJAX function), you can just check page === 0).

Demo: https://plnkr.co/edit/1j2mceV5ewKIxp9n

@caseyjhol
Copy link
Member Author

@NicolasCARPi This is still a WIP. I'll send it your way to review once I'm done.

@@ -1021,15 +1024,6 @@
});
});

this.fetchData(function () {
Copy link
Member Author

Choose a reason for hiding this comment

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

removing this duplicate block

@caseyjhol
Copy link
Member Author

@NicolasCARPi I'm still working on fixes for #2671, but there's no reason those should hold up this fix. Let me know your thoughts on dropping the source.load method.

@caseyjhol caseyjhol self-assigned this Feb 17, 2022
@caseyjhol caseyjhol added the bug label Feb 17, 2022
Copy link
Collaborator

@NicolasCARPi NicolasCARPi left a comment

Choose a reason for hiding this comment

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

I think it would be best to have a test case in the tests folder, with a fake ajax call (just a function returning stuff), so we can actually assess if the fix works and have different test case (you said the bug wasn't found before because it was only triggered in certain conditions).

We don't have automated tests (yet), but I believe it's important to add test cases when possible.

@caseyjhol
Copy link
Member Author

@NicolasCARPi Have you made any progress on implementing Cypress? I also have Cypress experience and might try to tackle some of the initial groundwork if you haven't already.

@NicolasCARPi
Copy link
Collaborator

Nope, I didn't, so feel free to lay the groundwork ;)

@caseyjhol caseyjhol marked this pull request as draft February 19, 2022 00:14
@caseyjhol caseyjhol changed the title Fix #2659: Fix ajax search Fix ajax search/add initial Cypress tests Feb 20, 2022
@caseyjhol caseyjhol removed the bug label Feb 20, 2022
@caseyjhol caseyjhol changed the title Fix ajax search/add initial Cypress tests Fix ajax search. Add initial Cypress tests Mar 16, 2022
@caseyjhol caseyjhol changed the title Fix ajax search. Add initial Cypress tests Fix ajax search and maxOptions. Add initial Cypress tests Mar 16, 2022
@caseyjhol caseyjhol marked this pull request as ready for review March 17, 2022 00:05
Copy link
Collaborator

@NicolasCARPi NicolasCARPi left a comment

Choose a reason for hiding this comment

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

I think it's great to finally have some end to end testing! This should be merged and a new beta version must be released ASAP :)

@caseyjhol caseyjhol merged commit 04cc1fd into dev Mar 17, 2022
@caseyjhol
Copy link
Member Author

@NicolasCARPi Yes it's about time. I'm excited to add more. Going to make it so much easier to add new features and fix bugs. Just need to update the changelog and I think we can prep for a release. I'd like to also look into it to see if there's a way to automatically create a nuget release at the same time as well.

@NicolasCARPi
Copy link
Collaborator

create a nuget release

sure, see: https://github.com/marketplace/actions/publish-nuget

Keep in mind that github actions have issues right now ;) https://www.githubstatus.com/

@NicolasCARPi NicolasCARPi deleted the 2659_fix-ajax-search branch March 18, 2022 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants