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

Implement search preview #1883

Closed
wants to merge 3 commits into from

Conversation

zachdaniel
Copy link
Contributor

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:

  1. The button that is displayed currently doesn't do anything on click.
  2. There is an issue I can't nail down with the iframe's height. Ideally, it should adapt to the height of its context.
  3. the right arrow should expand, and the left arrow should close, whereas currently they both just toggle.

Again, sorry I haven't had the time lately to get this over the line, but I think it is close enough to open a draft PR, hopefully someone has some cycles to wrap it up! If not, I can come back to it in maybe 4-5 weeks

const iframe = document.createElement('iframe')
const previewHref = elementToSelect.href.replace('.html', '.html?preview=true')
// The minimum permissions necessary for the iframe to run JavaScript and communicate with the parent window.
iframe.setAttribute('sandbox', 'allow-scripts allow-same-origin allow-popups allow-forms')
Copy link
Member

Choose a reason for hiding this comment

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

Quick question: is there a reason why you add allow-popups and allow-forms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC it was necessary for the iframe to render properly, but it was something I set early so we should explore removing it. I think allow-popups was to allow the hover info to work inside of previews.

@josevalim
Copy link
Member

@zachdaniel this looks really good to me! Besides your list of pending actions, something else we need to do is to use "pushMessage" to confirm that a preview is being rendered. Otherwise, we can link to old docs, that will render the whole page rather than a specific preview. You can see how we tackle this in: https://github.com/elixir-lang/ex_doc/blob/main/assets/js/tooltips/hints.js#L99

@zachdaniel
Copy link
Contributor Author

@josevalim that makes sense! I wasn't actually sure why that route was chosen, but I understand now. That will also likely solve the iframe height issues, if we just have it push all of its contents and then create an element, it won't have the iframe sizing problems :)

@josevalim josevalim mentioned this pull request Apr 3, 2024
3 tasks
@josevalim
Copy link
Member

I did a follow up with some refactorings on #1884. Mostly JS left now :D

@josevalim josevalim closed this Apr 3, 2024
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.

2 participants