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

[4.0] Fixing JS in com_finder #20888

Merged
merged 3 commits into from Jul 1, 2018
Merged

[4.0] Fixing JS in com_finder #20888

merged 3 commits into from Jul 1, 2018

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Jun 27, 2018

The JS in com_finder in 4.0 is very broken. There are several issues:

  • When typing in the input field, it looses focus very quickly, makeing input nearly impossible.
  • When typing quickly and then sending the form before the suggestions ajax request is fulfilled, an empty error message is shown shortly.
  • Suggestions are displayed for the previous term in the input field. So if you are typing the word "francais" and so far got "fra" and then press "n", the suggestions will retrieve the suggestions for "fra" and not for "fran". Likewise when deleting a character. So when you delete the "n" of "fran", you will get suggestions for "fran" instead.
  • Also sometimes the suggestions aren't displayed at all.

This PR fixes all of these things. I first updated the awesomplete suggestion script with the current head version from its repo (https://github.com/LeaVerou/awesomplete). The last release is from 2017 and we already have that in our codebase, which currently creates the focus errors.

I then created the awesomplete object just once and attached it to the input object, instead of creating it on each ajax call. Now we just update the list instead.

The event for the suggestions update is now upon "keyup", which then now takes the right string for the suggestions.

There is a bit of code that seems to be from a time where the placeholder attribute wasn't present. I removed that.

And last but not least, the error case for the ajax call first checks if there is any status code given. When we are just aborting the ajax call because we are submitting the form, then the status code is 0, otherwise it would be something like 200, 404 or 500.

Happy testing. Hope this makes you happy, @infograf768 and @carlitorweb 😉

@dgrammatiko
Copy link
Contributor

@Hackwar if you need to do changes on a vendor file you need to do them upstream!
This cannot be accepted because the npm install is removing the media/vendor folder and recreates it from the relative sources. So this is a no go!

Also the package.json dependencies have a format npm name: version so putting there some string is not gonna work.

Just do a npm i or node build.js --update and see what you are breaking here

@Hackwar
Copy link
Member Author

Hackwar commented Jun 27, 2018

package.json supports github repo links, which is what I did. I also did not modify the awesomplete files, but simply took the current head of their github repo. Considering that the last commit that I did to this PR is the result of npm install, I'm pretty confident with this. BTW: When doing an npm install on the current head of 4.0-dev, there is quite a few things that are being changed. So maybe you should again update this.

@infograf768
Copy link
Member

👍
Focus back and suggestions look fine with my first tests.

@infograf768
Copy link
Member

Now, just please solve the highlighting for non-ascii stuff. ;)

@Hackwar
Copy link
Member Author

Hackwar commented Jun 27, 2018

Did that in #20571 😉

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 011bbab


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20888.

@carlitorweb
Copy link
Member

nice one!!!

@carlitorweb
Copy link
Member

I have tested this item ✅ successfully on 011bbab


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20888.

@ghost
Copy link

ghost commented Jun 28, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 28, 2018
@wilsonge wilsonge merged commit d0f1aeb into joomla:4.0-dev Jul 1, 2018
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 1, 2018
@wilsonge wilsonge added this to the Joomla 4.0 milestone Jul 1, 2018
@Hackwar Hackwar deleted the j4finder_js branch April 27, 2019 08:10
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

6 participants