Skip to content

Search: define which element & event shows the modal #58

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

Merged
merged 4 commits into from
Apr 27, 2023
Merged

Search: define which element & event shows the modal #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e28931
Search: define which element & event shows the modal
humitos Apr 20, 2023
c59ed6f
Search: rename attributes and simplify logic
humitos Apr 25, 2023
dd4ec87
Lint
humitos Apr 25, 2023
fc3d5fd
pre-commit
humitos Apr 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
repos:
- repo: https://github.com/pre-commit/mirrors-prettier
rev: "v2.7.1"
hooks:
- id: prettier
args: ["--write", "src/**", "*.js", "*.json"]
7 changes: 6 additions & 1 deletion public/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@ <h2>Search</h2>

<p>Nothing to see here, just a placeholder to put the <code>readthedocs-search</code> tag.</p>
<p>Currently, it modifies the tag to be shown when pressing <code>\</code> (keycode 220) instead of <code>/</code> (keycode 191)</p>
<readthedocs-search show-modal-keycode="220"></readthedocs-search>
<readthedocs-search
trigger-keycode="220"
trigger-selector="#search-input"
></readthedocs-search>

<p>Clicking in the following input should show the search modal: <input id="search-input" placeholder="Search docs" /></p>

<h2>Notification</h2>

Expand Down
33 changes: 30 additions & 3 deletions src/search.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ export class SearchElement extends LitElement {
state: true,
},
cssFormFocusClasses: { state: true },
showModalKeycode: { type: Number, attribute: "show-modal-keycode" },
triggerKeycode: { type: Number, attribute: "trigger-keycode" },
triggerSelector: { type: String, attribute: "trigger-selector" },
triggerEvent: { type: String, attribute: "trigger-event" },
};

static styles = styleSheet;
Expand All @@ -64,7 +66,9 @@ export class SearchElement extends LitElement {
this.currentQueryRequest = null;
this.defaultFilter = null;
this.filters = [];
this.showModalKeycode = 191;
this.triggerKeycode = 191;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agjohnson should we have a trigger-modifier here or similar? See #57

How can we make people to use Ctrl + K for example to show the search?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I haven't done this, but it looks like we can get the modifier state from the keydown event passed into our event handler function.

I will say that we should be careful with defaults here though. It's easy to override native key bindings when working with modifier key presses.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fine to add a trigger-modifier-keycode attribute for this? 🤔

Copy link
Contributor

@agjohnson agjohnson Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, something like that, though I don't think keycode is the most correct. I believe this is the KeyboardEvent method you'd want to use:

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/getModifierState

So, more like <readthedocs-search trigger-key-modifier="control"> or something.

this.triggerSelector = null;
this.triggerEvent = "focusin";
}

loadConfig(config) {
Expand Down Expand Up @@ -483,20 +487,43 @@ export class SearchElement extends LitElement {
this.closeModal();
}
// Show the modal with `/`
else if (e.keyCode === this.showModalKeycode && !this.show) {
else if (e.keyCode === this.triggerKeycode && !this.show) {
// prevent opening "Quick Find" in Firefox
e.preventDefault();
this.showModal();
}
};

_handleShowModalUser = (e) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this isn't a normal class method instead of an anonymous function? It seems to be used as a method, given this.showModal()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. There is a reason for this,

https://github.com/readthedocs/readthedocs-client/blob/3e28931eb0eee6d198a04c9d43416dc5f03dceb6/src/search.js#L482-L483

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, that makes sense. I was going to suggest a static method, but I think in that scope this refers to the class itself.

e.preventDefault();
this.showModal();
};

connectedCallback() {
super.connectedCallback();
// open search modal if "forward slash" button is pressed
document.addEventListener("keydown", this._handleShowModal);

if (this.triggerSelector) {
let element = document.querySelector(this.triggerSelector);
if (element !== undefined) {
element.addEventListener(this.triggerEvent, this._handleShowModalUser);
}
}
}

disconnectedCallback() {
document.removeEventListener("keydown", this._handleShowModal);
if (this.triggerSelector) {
let element = document.querySelector(this.triggerSelector);
if (element !== undefined) {
element.removeEventListener(
this.triggerEvent,
this._handleShowModalUser
);
}
}

super.disconnectedCallback();
}
}
Expand Down