Updated search box shortcut and made it visible

[megaden]

Hi guys, this is supposed to fix #1112 - a small bit of help & my first contribution to OSS, so pls cut me some slack :)

Usage of complete_search_placeholder / localizable_search_placeholder is a bit controversial but saves updates to .po files. PLMK if you'd like to change the localizable string instead.

[pauloxnet]

LGTM

It would be nice if @adamchainz (I know he's very busy) also could review this PR as the author of the original issue #1112 confirming that the PR satisfies his original requests.

[megaden]

Can we make it command-k on macOS?

Tailwind does that but it's React-based, so I wonder if they figure it out client side. Anyways let me look into that, perhaps user agent header server side will be sufficient.

• [x] Try to replace shortcut with Command-K on MacOS

[adamchainz]

Anyways let me look into that, perhaps user agent header server side will be sufficient.

Please don't try use the User-Agent header, it's only robust to do it client side.

Should be possible to detect it and update the placeholder text in a small document ready handler.

[megaden]

@adamchainz Hey Adam, pls take a look at the latest version when you'll have a minute.

[pauloxnet]

@megaden thanks for your work.

I was thinking that given the shortness of this JavaScript file, could it make sense to rewrite it by removing the JQuery dependency and using a modern syntax (eg: let, const)?

[adamchainz]

Removing jQuery would be cool, but also deferrable. We've got a lot of jQuery in the project, it would be best to track rewriting to not use it in an issue - as we did for django.contrib.admin.

That said, I found a neat pure-JS example of the keyboard listener in the eslint codebase:

https://github.com/eslint/eslint/blob/d336cfc9145a72bf8730250ee1e331a135e6ee2c/docs/src/assets/js/search.js#L164-L168

The smooth scrolling into view seems like a nice addition.

[pauloxnet]

Removing jQuery would be cool, but also deferrable. We've got a lot of jQuery in the project, it would be best to track rewriting to not use it in an issue - as we did for django.contrib.admin.

The jQuery removal was only suggestion, because I saw how small is the JavaScript file modified in this PR, but it's also deferrable because I don't want to eventually block this issue to be merged only for this.

But if rewriting this file from scratch in pureJS require it less or equal time I would prefer this opportunity.

[megaden]

I aimed at achieving the goal with minimal updates hence tried to keep plumbing as is.

In regards to modern JS obviously I love it too 🙂 I assume you're happy with existing browser coverage since I haven't spot any postprocessing of JS (including production website).

In terms of jQuery removal I suggest to push that to a separate initiative since there's some plumbing missing now. For instance, to avoid messing with DOM readiness everywhere a dedicated RequireJS module could be used.

A bit of feedback from dev experience side of things which you may find useful:

• Docker Compose set up works great overall, kudos for it 👍

• While running tox I kept getting py38-tests failures complaining about "The SECRET_KEY setting must not be empty". Since SECRET_KEY is provided via env var in docker-compose.yml I tried to add SECRET_KEY to passenv in tox.ini but then it started to fail with "TypeError: can only concatenate str (not "NoneType") to str".

• While running tests via manage.py there also were errors (see pic below). It'll be a great deal for contributors using Docker Compose to ensure test suite works.

• I kept fighting whitespace in search-key.js due to .editorconfig:17 saying indent_style = tab while as far as I can tell files in "djangoproject/static/js/mod" have spaces rather than tabs.

Anyways let me know what do you think about current version.

[adamchainz]

• I kept fighting whitespace in search-key.js due to .editorconfig:17 saying indent_style = tab while as far as I can tell files in "djangoproject/static/js/mod" have spaces rather than tabs.

I think we should make an issue to update the editorconfig, and stop using tabs. Perhaps we could sync with django's editorconfig?

[pauloxnet]

• I kept fighting whitespace in search-key.js due to .editorconfig:17 saying indent_style = tab while as far as I can tell files in "djangoproject/static/js/mod" have spaces rather than tabs.

I think we should make an issue to update the editorconfig, and stop using tabs. Perhaps we could sync with django's editorconfig?

I agree with that feel free to open an issue and proposed a fix to the .editorconfig or .tox.ini

[pauloxnet]

I aimed at achieving the goal with minimal updates hence tried to keep plumbing as is.

In regards to modern JS obviously I love it too slightly_smiling_face I assume you're happy with existing browser coverage since I haven't spot any postprocessing of JS (including production website).

In terms of jQuery removal I suggest to push that to a separate initiative since there's some plumbing missing now. For instance, to avoid messing with DOM readiness everywhere a dedicated RequireJS module could be used.

Ok, I agree with what you say. I leave @adamchainz the revision of the latest changes.

[megaden]

Hey guys, any luck with this getting merged? It was up in the air for a while already.

[pauloxnet]

Hey guys, any luck with this getting merged? It was up in the air for a while already.

I was waiting for a review from @adamchainz who is the creator of the issue, but I try to review again too.

P.S. next time try to use a more inclusive salutation form. See https://heyguys.cc/

[pauloxnet]

I approved the PR but I ask you to way some other days to let @adamchainz to review the code (I think he is a bit busy this days).

[megaden]

Thanks, Paolo! Sure, whenever it'd be possible to pick this up.

[pauloxnet]

@megaden at this point if you can rebase this PR we can merge it. @valentinogagliardi thanks for your help

[megaden]

@pauloxnet I synced my fork & rebased the branch on top of the latest commit to main.

[pauloxnet]

@pauloxnet I synced my fork & rebased the branch on top of the latest commit to main.

Unfortunately tox is failing

[megaden]

Unfortunately tox is failing

The same happens for the tip of the main branch - https://github.com/django/djangoproject.com/actions/runs/3650268494/jobs/6166491637 - when it'll be sorted I can rebase once again.

[pauloxnet]

The same happens for the tip of the main branch - https://github.com/django/djangoproject.com/actions/runs/3650268494/jobs/6166491637 - when it'll be sorted I can rebase once again.

@megaden tox was not pinned and since tox 4 was published, the tests have started to fail.

I marked tox<4 in the main branch.

If you managed to make the last rebase I'm sure that we will finally be able to merge this PR.

[megaden]

@pauloxnet I rebased the branch, could you please approve the workflow, so we'll ensure everything is good now?

[pauloxnet]

@pauloxnet I rebased the branch, could you please approve the workflow, so we'll ensure everything is good now?

Finally it worked, I'm going to squash and merge this PR.