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.

Sign up for GitHub

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

Jump to bottom

Fix type error in console when searching #2740

Closed
wants to merge 5 commits into from
Closed

Fix type error in console when searching #2740

wants to merge 5 commits into from

Conversation

MarijnvSprundel
Copy link

Description of the Change

Add a check if currentindex is undefined then break, this prevents a error briefly appearing in the console.

Closes #2725

Alternate Designs

None, no designs were made.

Possible Drawbacks

None, just a quick check.

Verification Process

Well firstly I got tasked to fix this bug on the website, which quickly led me to the autosuggest.js. But this process was painful since the minified version was throwing the errors, so I had to dig trough a one line file.

The fix I currently have implemented inserts this line of code between the other lines using regex, but it would be better for this fix to be in the plugin itself.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

  • Fixed a brief type error in the console

Credits

Props @MarijnvSprundel

@felipeelia
Copy link
Member

Hi @MarijnvSprundel, thanks for Pull Request! Do you mind addressing the code standard adjustment highlighted by the automatic check so we can have this one merged?

@felipeelia
Copy link
Member

Hi @MarijnvSprundel, giving the PR a try, the error is still happening even with the code in place. That makes sense, as the code is checking for currentIndex after it was already used in results[currentIndex]. We could move up that if you added, making it like:

if (typeof currentIndex === 'undefined') {
	break;
}
if (results[currentIndex].classList.contains('selected')) {
	// navigate to the item defined in the span's data-url attribute
	selectItem(input, results[currentIndex].querySelector('.autosuggest-link'));
}

but that is not necessary, as we can make use of the optional chaining feature instead. With that, the only change we would need to fix the problem is to replace

if (results[currentIndex].classList.contains('selected')) {

with

if (results[currentIndex]?.classList.contains('selected')) {

Do you mind changing the PR to look like that? Thanks!

@felipeelia
Copy link
Member

Hi @MarijnvSprundel, reviewing this one again and saw b493f32. Any reason why you decided not to use optional chaining instead of the if?

@felipeelia felipeelia added this to the 4.2.0 milestone May 10, 2022
@MarijnvSprundel
Copy link
Author

I tested it with the optional chaining but it still seemed to throw the error, but maybe I implemented it the wrong way. Feel free to change it.

@felipeelia
Copy link
Member

Closing in favor of #2754.

@felipeelia felipeelia closed this May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@burhandodhy burhandodhy

Labels
None yet
Projects
None yet
Milestone
4.2.0
Development

Successfully merging this pull request may close these issues.

BUG: Type error in console when searching
3 participants
@MarijnvSprundel @felipeelia @burhandodhy