Updated search box shortcut and made it visible #1236

megaden · 2022-10-12T14:08:23Z

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 · 2022-10-12T14:14:29Z

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.

adamchainz

Can we make it command-k on macOS?

djangoproject/static/js/mod/search-key.js

megaden · 2022-10-12T15:06:51Z

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.

Try to replace shortcut with Command-K on MacOS

emorozov · 2022-10-12T15:23:02Z

docs/forms.py

    def __init__(self, data=None, **kwargs):
        self.release = kwargs.pop('release')
        super().__init__(data=data, **kwargs)
+        complete_search_placeholder = '%s (Ctrl + K)' % (localizable_search_placeholder % self.release.human_version)


Nit: two string interpolations in one line look weird and error-prone, I'd split this in two statements or used two different ways of interpolation (f-string and % interpolation, or format() and % interpolation).

Agreed but in the latest version we do everything client side, so doesn't matter anymore.

adamchainz · 2022-10-12T15:26:29Z

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 · 2022-10-12T19:27:02Z

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

pauloxnet · 2022-10-12T21:05:30Z

@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 · 2022-10-12T22:06:05Z

djangoproject/static/js/mod/search-key.js

+                search_form_input.attr('placeholder', `${raw_placeholder} (${kbd_shortcut_suffix})`);
+
+                $(window).keydown(function(e) {
+                    if (e.key === 'k' && e.ctrlKey && $('input:focus, textarea:focus').length === 0) {


we need to check for metaKey on macOS, rather than ctrlKey, to detect “Command”: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/metaKey

adamchainz · 2022-10-12T22:22:04Z

djangoproject/static/js/mod/search-key.js

+                var kbd_shortcut_suffix = "Ctrl + K";
+                if (navigator.userAgent.indexOf("Mac") !== -1) kbd_shortcut_suffix = "⌘ + K";
+                search_form_input.attr('placeholder', `${raw_placeholder} (${kbd_shortcut_suffix})`);


It's more idiomatic in JavaScript to use the ternary operator for such conditional assignments:

Suggested change

var kbd_shortcut_suffix = "Ctrl + K";

if (navigator.userAgent.indexOf("Mac") !== -1) kbd_shortcut_suffix = "⌘ + K";

search_form_input.attr('placeholder', `${raw_placeholder} (${kbd_shortcut_suffix})`);

const shortcut = (navigator.userAgent.indexOf("Mac") !== -1) ? "⌘ + K" : "Ctrl + K";

search_form_input.attr('placeholder', `${raw_placeholder} (${kbd_shortcut_suffix})`);

This also allows us to use const to declare that the variable will not change.

Also there's no need to use a verbose variable name when the variable has a single use on the next line 😉

adamchainz · 2022-10-12T22:22:50Z

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 · 2022-10-12T22:33:09Z

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 · 2022-10-13T08:59:39Z

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 · 2022-10-13T09:43:18Z

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 · 2022-10-13T10:34:51Z

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 · 2022-10-13T10:47:59Z

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 · 2022-11-04T12:46:00Z

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

pauloxnet · 2022-11-07T12:04:23Z

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 · 2022-11-07T16:18:59Z

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 · 2022-11-07T16:22:54Z

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

valentinogagliardi

Hi! @pauloxnet asked me to take a quick look at this PR. Looks good to me. Thanks!

pauloxnet · 2022-12-22T15:53:02Z

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

megaden · 2022-12-22T16:14:21Z

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

pauloxnet · 2022-12-22T16:43:42Z

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

Unfortunately tox is failing

megaden · 2022-12-22T16:48:05Z

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 · 2022-12-23T09:49:27Z

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.

Co-authored-by: Adam Johnson <me@adamj.eu>

megaden · 2022-12-23T10:00:16Z

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

pauloxnet · 2022-12-23T10:03:11Z

@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.

Current logic incorrectly shows "⌘ + K" in Chrome on Windows (user agent 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/110.0.0.0 Safari/537.36'). It does not contain "Mac". The logic was introduced in django#1236 and appears to be swapped - showing Windows/Linux shortcut on mac and vice-versa.

Current logic incorrectly shows "⌘ + K" in Chrome on Windows (user agent 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/110.0.0.0 Safari/537.36'). It does not contain "Mac". The logic was introduced in #1236 and appears to be swapped - showing Windows/Linux shortcut on mac and vice-versa.

megaden mentioned this pull request Oct 12, 2022

Change searchbox shortcut to ctrl-k/command-k, and make it visible #1112

Closed

pauloxnet added accessibility docs labels Oct 12, 2022

adamchainz reviewed Oct 12, 2022

View reviewed changes

djangoproject/static/js/mod/search-key.js Outdated Show resolved Hide resolved

emorozov reviewed Oct 12, 2022

View reviewed changes

pauloxnet added the search label Oct 12, 2022

adamchainz reviewed Oct 12, 2022

View reviewed changes

pauloxnet approved these changes Nov 7, 2022

View reviewed changes

valentinogagliardi approved these changes Dec 22, 2022

View reviewed changes

megaden force-pushed the new-searchbox-shortcut branch from c05c2f6 to 036e484 Compare December 22, 2022 16:12

megaden and others added 5 commits December 23, 2022 12:58

Updated search box shortcut and made it visible.

5f81e7b

Rearranged checks to do cheaper ones first.

79c4638

Co-authored-by: Adam Johnson <me@adamj.eu>

Adding platform-specific shortcut to input placeholder client side.

f61b8f7

Fixed whitespace.

144731a

A few minor tweaks.

633dfd0

megaden force-pushed the new-searchbox-shortcut branch from 036e484 to 633dfd0 Compare December 23, 2022 09:58

pauloxnet merged commit b734015 into django:main Dec 23, 2022

megaden deleted the new-searchbox-shortcut branch December 23, 2022 11:08

mick88 mentioned this pull request Feb 11, 2023

Fix logic for selecting search key string #1312

Merged

medmunds mentioned this pull request Feb 13, 2025

Restore / shortcut for searchbox? #1946

Closed

Uh oh!

Updated search box shortcut and made it visible #1236

Updated search box shortcut and made it visible #1236

Uh oh!

Conversation

megaden commented Oct 12, 2022

Uh oh!

pauloxnet commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamchainz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

megaden commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emorozov Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

megaden Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

adamchainz commented Oct 12, 2022

Uh oh!

megaden commented Oct 12, 2022

Uh oh!

pauloxnet commented Oct 12, 2022

Uh oh!

adamchainz Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

adamchainz Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

adamchainz commented Oct 12, 2022

Uh oh!

pauloxnet commented Oct 12, 2022

Uh oh!

megaden commented Oct 13, 2022

Uh oh!

adamchainz commented Oct 13, 2022

Uh oh!

pauloxnet commented Oct 13, 2022

Uh oh!

pauloxnet commented Oct 13, 2022

Uh oh!

megaden commented Nov 4, 2022

Uh oh!

pauloxnet commented Nov 7, 2022

Uh oh!

pauloxnet commented Nov 7, 2022

Uh oh!

megaden commented Nov 7, 2022

Uh oh!

valentinogagliardi left a comment

Choose a reason for hiding this comment

Uh oh!

pauloxnet commented Dec 22, 2022

Uh oh!

megaden commented Dec 22, 2022

Uh oh!

pauloxnet commented Dec 22, 2022

Uh oh!

megaden commented Dec 22, 2022

Uh oh!

pauloxnet commented Dec 23, 2022

Uh oh!

megaden commented Dec 23, 2022

Uh oh!

pauloxnet commented Dec 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pauloxnet commented Oct 12, 2022 •

edited

Loading

megaden commented Oct 12, 2022 •

edited

Loading