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

Search: doesn't open after closing by clicking outside #169

Closed
humitos opened this issue Oct 23, 2023 · 4 comments · Fixed by #299
Closed

Search: doesn't open after closing by clicking outside #169

humitos opened this issue Oct 23, 2023 · 4 comments · Fixed by #299
Assignees
Labels
Accepted Accepted issue on our roadmap Bug A bug

Comments

@humitos
Copy link
Member

humitos commented Oct 23, 2023

Steps to reproduce:

  • hit / to open the search modal
  • click outside the modal to close it
  • hit / again
  • it does not open as expected

This is because the state saved inside the hotkeys.js file is not updated when clicking outside.

@humitos humitos added Bug A bug Accepted Accepted issue on our roadmap labels Oct 23, 2023
@github-project-automation github-project-automation bot moved this to Planned in 📍Roadmap Oct 23, 2023
@humitos humitos assigned humitos and unassigned humitos Oct 23, 2023
@humitos
Copy link
Member Author

humitos commented Oct 23, 2023

I suppose that the action "clicking outside the modal" (at https://github.com/readthedocs/readthedocs-client/blob/6ecc7525c3ce085e69ae515fa6c592011633396c/src/search.js#L113) should trigger the event EVENT_READTHEDOCS_SEARCH_HIDE that should be catched by the hotkeys.js addon and update its state regarding that.

Another alternative could be a way to globally check if the readthedocs-search tag is visible or by inspecting the DOM.

@humitos
Copy link
Member Author

humitos commented Mar 5, 2024

I did something similar in #247 if you want to have a related example for this.

@zanderle
Copy link
Collaborator

the action "clicking outside the modal" should trigger the event EVENT_READTHEDOCS_SEARCH_HIDE that should be catched by the hotkeys.js addon and update its state regarding that.
Another alternative could be a way to globally check if the readthedocs-search tag is visible or by inspecting the DOM.

Those are both valid solutions. However I'm wondering if we even need the requirement that the same hotkey both opens AND closes the modal? Since we already use Escape to close the modal, do we need to close it with / as well? If not, we could have hotkeys.js always fire EVENT_READTHEDOCS_SEARCH_SHOW when the hotkey is pressed, and let the search modal simply ignore the event, if it's already opened.

@humitos
Copy link
Member Author

humitos commented Apr 3, 2024

@zanderle yeah, we don't need to use the same key to open and close. / should open it and Esc should close it. Ideally, we should use the EVENT_ everywhere to open/close the modal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Bug A bug
Projects
Status: Done
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants