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 preview #1884

Merged
merged 14 commits into from
Apr 8, 2024
Merged

Search preview #1884

merged 14 commits into from
Apr 8, 2024

Conversation

josevalim
Copy link
Member

@josevalim josevalim commented Apr 3, 2024

Continuation of #1883. Quoting @zachdaniel:

Hey all!

I've discussed the ability to preview pages in autocomplete with @josevalim, and this is the groundwork for that feature. There is more to be done, and I hate to be one to throw things over the fence, but with Ash's 3.0 release coming soon, multiple trainings ahead, and various other things on my plate, I just haven't had time to wrap this up and I don't know when I will have time to get back to it. Additionally, this isn't my wheelhouse so it is possible that there are folks who are more familiar w/ making good UIs that could be more effective. Here is a video showing the current behavior:

CleanShot.2024-04-03.at.00.00.16.mp4

And here is what remains to be done:

  • Fix the left and right arrow navigation. The right arrow should expand, and the left arrow should close, whereas currently they both just toggle

  • The buttons displayed doesn't do anything on click

  • Use postMessage. The iframe should start as hidden and we should only show the iframe once it uses postMessage. The postMessage should include the content height, so we can render the maximum iframe height accordingly

@josevalim
Copy link
Member Author

Most of the pending changes are related to JavaScript, so we would really appreciate help if someone who is interested and can do well with JavaScript could pick the remaining tasks up. Thank you!

@webspaceadam
Copy link

webspaceadam commented Apr 4, 2024

Is there a timeline for this, or "just" asap? I implemented a keynavigation a few months ago at work. Maybe i can look into it. But don't know when i will find time this week. Will look into it when i find some spare time!

@josevalim
Copy link
Member Author

We have other features that will build on top of this, so it would be nice to ship it sooner than later, but it depends on contributions. You could also contribute part of it (keyboard navigation) by sending a PR to this branch and someone else tackles the rest. :) Thanks!

@mudssrali
Copy link

We have other features that will build on top of this, so it would be nice to ship it sooner than later, but it depends on contributions. You could also contribute part of it (keyboard navigation) by sending a PR to this branch and someone else tackles the rest. :) Thanks!

I can take it if nobody is already starting working on it. @josevalim

@josevalim
Copy link
Member Author

@mudssrali please go ahead!

…d, and the left arrow should close, whereas currently they both just toggle
@ThaddeusJiang ThaddeusJiang mentioned this pull request Apr 4, 2024
3 tasks
@mudssrali
Copy link

@josevalim - I just started working on it and realized that @ThaddeusJiang already made good progress (#1885).

@josevalim
Copy link
Member Author

Thank you @ThaddeusJiang and @arathunku. You both brought good suggestions which I have incorporated. I changed the open/close preview button a bit, the markup right now is not great, but I can fix that later.

We just need to adjust the iframe width now and use postMessage. @ThaddeusJiang, would you be interested in tackling that as well?

Thank you all ❤️

@mudssrali
Copy link

@josevalim how about this minor change, instead of Autocompletion results for: , we can say Search suggestions for:

image

@ThaddeusJiang
Copy link
Contributor

Thank you @ThaddeusJiang and @arathunku. You both brought good suggestions which I have incorporated. I changed the open/close preview button a bit, the markup right now is not great, but I can fix that later.

We just need to adjust the iframe width now and use postMessage. @ThaddeusJiang, would you be interested in tackling that as well?

Thank you all ❤️

@josevalim Yes, I will try it. 😎

@zachdaniel
Copy link
Contributor

Oh, I also had a thought about how we can leverage these previews. Currently, we show docs on hover for functions/modules. I think we can use the code I wrote to extract out content from a heading to the next relevant heading in previews to expand the "hints" interface to guides. In the same way that anything is previewable, we can make anything "hintable" also.

@josevalim
Copy link
Member Author

I have pushed more styling and I am making sure the theme propagates. I think as soon as the iframe postMessage and height are addressed, we are ready to go!

@zachdaniel
Copy link
Contributor

One more thing: we need to set some kind of metadata in the HTML that indicates that this is a proper page preview. The other use cases discussed for this feature need to be able to know potentially without running js whether or not they are looking at a preview or just the entire docs page because ex_doc isn't up to date for that dependency.

@josevalim
Copy link
Member Author

It is a static file, so it is hard to anything to metadata after the fact without JS. It may be easier to discuss if there is a concrete use case to give as an example, but let's be careful to also not side-track this with orthogonal concerns. :)

@zachdaniel
Copy link
Contributor

Both good points 👍

@ThaddeusJiang
Copy link
Contributor

I am going to implement postMessage for sync iframe height

@josevalim
Copy link
Member Author

Thank you @zachdaniel, @ThaddeusJiang and @arathunku! I believe this is ready to be shipped. Feel free to review and drop any last comments or PRs. :)

@arathunku
Copy link
Contributor

Awesome work all of you! This is great!

Peek.2024-04-08.13-32.mp4

@josevalim
Copy link
Member Author

@arathunku I have noticed that the text of "Open preview" in your case is not aligned with the icon. Are you using a zoom or similar? If not, does it work if you add a line-height of 14px or similar to .autocomplete-preview-indicator button {?

@arathunku
Copy link
Contributor

@josevalim great guess, I'm using zoom at Firefox @ 120%. Is that something closer to what you see/expect?

image

@zachdaniel
Copy link
Contributor

Let's gooooo 🥰

@josevalim
Copy link
Member Author

@arathunku Yes, thank you!

@josevalim josevalim merged commit 0e076ca into main Apr 8, 2024
8 checks passed
@josevalim
Copy link
Member Author

💚 💙 💜 💛 ❤️

@josevalim josevalim deleted the search-preview branch April 8, 2024 12:48
@garazdawi
Copy link
Contributor

garazdawi commented Apr 8, 2024

I just tested this and when previewing something that is very large the autocomplete box goes offscreen and an unnessary(?) scrollbar is shown. For example using Chrome:

Screencast.from.2024-04-08.15.25.04.webm

Speaking of previewing very large functions... when hovering over a link only the summary is shown instead of the entire thing. Was doing the same thing for this considered?

@zachdaniel
Copy link
Contributor

I think ideally search preview should be capped at the height of the screen, and then you'd scroll within it. I do think it's important to show everything here (not a small summary), as the idea is to let you avoid having to visit the page if the information you need is there. This coupled with some other changes that I believe are underway or completed to summon the search bar wherever you currently are at in the scroll basically allows you to browse docs without having to jump around or leave the place you're at.

@josevalim
Copy link
Member Author

@garazdawi I have fixed the inner scrollbar, thanks for trying it out!

@josevalim
Copy link
Member Author

@garazdawi I think we can consider reusing the preview system for the hints, I just haven't put too much thought on it at the moment.

@josevalim
Copy link
Member Author

I found a bug. If we resize the window (for example, by making it thinner), we don't recompute the new iframe height. @ThaddeusJiang, would you like to take a look at it? We can either listen to changes in the iframe OR have the iframe post a new type of "preview-resize" message? Thank you!

@garazdawi
Copy link
Contributor

I think we can consider reusing the preview system for the hints, I just haven't put too much thought on it at the moment.

@josevalim for the hints it will be a hard balance to strike. I've often found that there are times that I would like to see more information, like when hovering over a type link, I would like to see what the fields of the type are. But also when hovering over erlang:system_info/1, I do not want to see the entire docs as that is many many pages long. Maybe it is possible to find some good balance by truncating very long specs/docs?

@ThaddeusJiang
Copy link
Contributor

I found a bug. If we resize the window (for example, by making it thinner), we don't recompute the new iframe height. @ThaddeusJiang, would you like to take a look at it? We can either listen to changes in the iframe OR have the iframe post a new type of "preview-resize" message? Thank you!

@josevalim Yes, I will do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants