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

enhancement: add 2 improvements to local search #1618

Merged
merged 4 commits into from
Apr 24, 2017

Conversation

uchuhimo
Copy link
Contributor

@uchuhimo uchuhimo commented Apr 24, 2017

Improvements:

  • Add a new flag local_search.trigger. If set to manual, Search will be triggered by pressing enter key or search button; If set to auto, Search will be triggered by changing input (It's the default behavior, but suffer from lagging when containing many posts in site).
  • Now results containing whole search text take precedence over other results, and articles containing more whole search texts take precedence over other articles. E.g., if you search "you can", results containing "you can" will be shown before results only containing "you" or "can".

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Apr 24, 2017

@uchuhimo need to set auto to default trigger.

image
All work how need. Profi.

@ivan-nginx
Copy link
Collaborator

@uchuhimo nice and Good Job!

@ivan-nginx ivan-nginx merged commit 3d33c9a into iissnan:master Apr 24, 2017
@uchuhimo uchuhimo deleted the feature-local-search branch April 24, 2017 11:28
@ivan-nginx
Copy link
Collaborator

@uchuhimo find 1 little bug — esc key not working: fa39fda

@uchuhimo
Copy link
Contributor Author

@ivan-nginx I'm a little confused. I cannot find this commit on master. I check the related issue, I find it is pushed into liszd/hexo-theme-next and habren/hexo-theme-next, but not iissnan/hexo-theme-next. What does it means? Is it a feature that will add to master in future? Do I miss anything?

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Apr 24, 2017

This was add by iissnan (esc key search box). I don't know why u cannot find it, try to this link (Feature: Close Search Popup by pressing ESC key. #1467).

I cannot find this commit on master.

How it may be? Commit on master, i can make screens. Also, try to follow layout/_partials/search/localsearch.swig history of file change, u must see all commits.

@uchuhimo also, try to other OS or virtual box or ask for friends to check this link with this commit. The branch is always master.

@uchuhimo
Copy link
Contributor Author

@ivan-nginx I have tested it. Esc key will hide search panel, but never clean the search input and search result. Is it what you observe?

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Apr 24, 2017

@uchuhimo yes, it is. I don't use this Esc key, but i remember what it was added and worked and for now it not worked. I think, need to add Esc key to your event listener:

          if ('auto' === '{{ theme.local_search.trigger }}') {
             input.addEventListener('input', inputEventFunction);
           } else {
             $('.search-icon').click(inputEventFunction);
             input.addEventListener('keypress', function (event) {
               if (event.keyCode === 13) {
                 inputEventFunction();
               }
             });
           }

It's 43, if i right remebmer.
Added: oh, no, it's 27)

@uchuhimo
Copy link
Contributor Author

@ivan-nginx I have fixed it, check #1621

@ivan-nginx
Copy link
Collaborator

@uchuhimo hi!
Look, there is 1 bug and 1 improvement for local search:

  • W3 validator give some errors: Attribute autocapitalize not allowed on element input at this point.
    <div class="local-search-input-wrapper">
      <input autocapitalize="off" autocomplete="off" autocorrect="off" placeholder="Поиск..." spellcheck="false" type="text" id="local-search-input">
    </div>
  • It is possible to do something like wait, search is now loading if i click on search and it's load for 5-10 seconds because there is big data in search.xml. Something like popup loading, anywhat.

@uchuhimo
Copy link
Contributor Author

@ivan-nginx

W3 validator give some errors: Attribute autocapitalize not allowed on element input at this point.

autocapitalize is a nonstandard attribute of input, only supported by Chrome and Safari. W3 validator also shows error "Attribute autocorrect not allowed on element input at this point." in my test, because autocorrect is a nonstandard attribute only supported by Safari.

Both of these attributes are introduced in 27eaf46 by @Acris . I don't know the best practice to handle such a browser compatibility problem. Should I delete them? Or just leave them there because browser will ignore them if not support them?

Any idea? @ivan-nginx @Acris

@ivan-nginx
Copy link
Collaborator

@uchuhimo this attributs need in searchbox? For what need autocorrect or autocapitalize? If they not needed, need to delete (or comment) them, i think.

@uchuhimo
Copy link
Contributor Author

@ivan-nginx Both of these attributes are set to false, so I guess they are not needed. But I want to know why @Acris introduces them before reaching a conclusion.

@uchuhimo
Copy link
Contributor Author

@ivan-nginx

It is possible to do something like wait, search is now loading if i click on search and it's load for 5-10 seconds because there is big data in search.xml. Something like popup loading, anywhat.

I add a loading animation, please check #1633

@ivan-nginx
Copy link
Collaborator

@Acris what u say about bugs in W3?

@ivan-nginx ivan-nginx added the Bug label Apr 30, 2017
@Acris
Copy link
Collaborator

Acris commented Apr 30, 2017

Hi, this two attributes is only for mobile users, you can find more details on MDN: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input

@ivan-nginx
Copy link
Collaborator

@Acris and? Errors in W3 still present. Don't care about it? Need to do something, what u think?

@Acris
Copy link
Collaborator

Acris commented Apr 30, 2017

Yes, because autocorrect is a Non-standard attribute . I think we can ignore this error, or, just delete this attribute.

@ivan-nginx
Copy link
Collaborator

@Acris ignore? I think we cannot ignor any errors. Acris, u not ignore 0.5 pixels in scroll to top, remeber it?) And now u offer to ignore W3 global errors? I dont't belive in this. Need to fix it by anyway.

@Acris
Copy link
Collaborator

Acris commented Apr 30, 2017

Well, I'll fix it when I have time (maybe tomorrow).

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Apr 30, 2017

@Acris ok ok, do not need hurry. But not need ignore this errors too. This is global errors wich are do affect on SEO. And SEO is holy. ;)


Also, can u reg in gitter too? This cool chat and may resolve any problems very fast there.

@Acris
Copy link
Collaborator

Acris commented May 2, 2017

@ivan-nginx I've fixed it.

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented May 2, 2017

Thank you!
@Acris but in W3 still 2 errors: https://validator.w3.org/nu/?doc=https%3A%2F%2Falmostover.ru%2F

@Acris
Copy link
Collaborator

Acris commented May 3, 2017

@ivan-nginx Are u updated latest code?

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented May 3, 2017

@Acris update was from this patch: 43d37ff

I don't understand: i checked your site and there is no errors for this.

Ok, there is another commit from you: 9fe2637
I don't find it firstly because u do separetaded commits without pull request. :)

Bug is solved.

@ivan-nginx ivan-nginx removed the Bug label May 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants