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

Fullscreen search modal #18

Closed
wants to merge 10 commits into from
Closed

Conversation

phonzammi
Copy link
Collaborator

No description provided.

@brillout
Copy link
Owner

FYI I created a new directory docsearch/. How about we move the CSS over there?

@brillout brillout force-pushed the phonzammi/full-screen-search branch from 3a25cd7 to 13058ca Compare October 24, 2024 11:15
@brillout
Copy link
Owner

I just rebased and force-pushed, I hope it's ok. We can now try the changes by using Docpress's demo: 617f339.

@brillout
Copy link
Owner

Let me implement the <meta> thing, so that we can assign a categoy to each page. Let's then do a column layout for the docsearch modal.

@phonzammi phonzammi force-pushed the phonzammi/full-screen-search branch from 13058ca to 6a0257e Compare October 25, 2024 08:29
@phonzammi
Copy link
Collaborator Author

FYI I created a new directory docsearch/. How about we move the CSS over there?

Done. I’ve also made a few improvements, please take a look.

@brillout
Copy link
Owner

Nice, much better! I think we can polish a bit more: removing the rounded corner + avoiding the scrolling of the main view.

The docsearch category meta tag is live so I think we can now implement the column layout: every page is guaranteed to have a category (it falls back to Miscellaneous). I expect it to be a bit tricky if there are too many columns that don't fit the viewport. Let's try something simple first and let's maybe do something more sophisticated if we aren't happy with it. (In case you're curious, you can see how <MenuModal> does it: see getStyleColumnLayout.ts.)

@phonzammi
Copy link
Collaborator Author

avoiding the scrolling of the main view

Algolia DocSearch has already added this functionality by toggling the DocSearch--active class on the <body> tag, but it doesn’t seem to be working

@brillout
Copy link
Owner

brillout commented Oct 27, 2024 via email

@phonzammi
Copy link
Collaborator Author

phonzammi commented Oct 27, 2024

I tried to follow the approach you used with MenuModal, but the issue with DocSearchModal is that when we click the Cancel button, the class doesn’t get removed.

The DocSearchModal & it's children (e.g. the Cancel button) are created dynamically, I don't know how we can add an event listener to the Cancel button

@brillout
Copy link
Owner

brillout commented Oct 27, 2024 via email

@phonzammi
Copy link
Collaborator Author

I think we can use our keydown listener to add our own CSS class to html.

Yes, we can and it works to toggle the class.
but the issue with DocSearchModal is that when we click the Cancel button, the class doesn’t get removed.

@phonzammi
Copy link
Collaborator Author

phonzammi commented Oct 29, 2024

@brillout, Please take a look at my latest commits

if (!isClosed()) return
toggle()
if (isButton) {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd refactor this a bit; it's a bit difficult to understand. How about onDocsearchModalOpen() instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When a user opens the docsearch modal using the hotkeys, we only need to add the class, we don't need to call

window.dispatchEvent(new KeyboardEvent('keydown', { key: 'k', ctrlKey: true }))

because useDocSearchKeyboardEvents() handles that.
Otherwise, the browser will throw an error : Uncaught RangeError: Maximum call stack size exceeded in the console.

Copy link
Owner

Choose a reason for hiding this comment

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

Yea, the logic seems accurate. What I meant is to refactor it to make it a little bit less convoluted.

if (isButton) {
window.dispatchEvent(new KeyboardEvent('keydown', { key: 'k', ctrlKey: true }))
}
document.documentElement.classList.add('DocSearch--active')
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add a comment about this.

@@ -5,24 +5,31 @@ import { assert } from '../utils/client'

function closeDocsearchModal() {
if (isClosed()) return
toggle()
document.documentElement.classList.remove('DocSearch--active')
Copy link
Owner

Choose a reason for hiding this comment

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

I'd refactor this a bit; let's collocate this line next to its add() counterpart.


function isClosed() {
const test1 = !document.body.classList.contains('DocSearch--active')
const test1 =
Copy link
Owner

Choose a reason for hiding this comment

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

I would use an assert() instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like this : 993b47c ?

@brillout
Copy link
Owner

Nice, looks it's working 💯

@brillout
Copy link
Owner

brillout commented Oct 30, 2024

I just tried, really nice.

The layout seems a bit broken sometimes, for example:

image

@brillout brillout closed this Dec 17, 2024
@brillout brillout mentioned this pull request Feb 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants