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

fix EmbeddableDisplay #2632

Merged
merged 4 commits into from
Aug 27, 2023
Merged

fix EmbeddableDisplay #2632

merged 4 commits into from
Aug 27, 2023

Conversation

jbrea
Copy link
Contributor

@jbrea jbrea commented Aug 25, 2023

Fixes #2619

@github-actions
Copy link
Contributor

Try this Pull Request!

Open Julia and type:

julia> import Pkg
julia> Pkg.activate(temp=true)
julia> Pkg.add(url="https://github.com/jbrea/Pluto.jl", rev="fix_more")
julia> using Pluto

@disberd
Copy link
Contributor

disberd commented Aug 26, 2023

I just tried out this pull request but unfortunately it does not seem to fix #2619 for logs. more inside @info does not seem to work in any situation (with or without embed_display and with or without @htl).
This was expected as @Pangoraw pointed out in his comment

@jbrea
Copy link
Contributor Author

jbrea commented Aug 26, 2023

Ah, yes, indeed. I only looked at my original bug report. This is the only functionality I need for the class starting mid September.
If I find the time I can try to have a look at @info and friends.

@jbrea
Copy link
Contributor Author

jbrea commented Aug 26, 2023

This takes too much time for me, sorry. I fixed one issue, which was that all logs had a hand-written cell-id "adsf".
But now I would have to mess with set_output! in Run.jl and I don't yet really see the best solution. Maybe it would be best to treat logs as special published_outputs? Or copy the mechanism of published_outputs to logs? No clue, and I don't have the time to figure it out and fix this this now.

@disberd
Copy link
Contributor

disberd commented Aug 26, 2023

Yeah don't worry I'll just open a new issue for logs.
I think you can also just revert your latest commit so that you only fix the lingering error in embed display.
That way it's really just a single line fixing a specific and clearly identified issue. Hopefully that way it will be much faster to get merged :)

@jbrea
Copy link
Contributor Author

jbrea commented Aug 26, 2023

OK, thanks.

@disberd
Copy link
Contributor

disberd commented Aug 26, 2023

Tagging @Pangoraw for review/merge. This is important to get merged before the next release as embed_display will break since #2608 has been merged (and missed changing the code of show in this fix)

@Pangoraw
Copy link
Collaborator

Thanks @jbrea and @disberd! I added a test which could have detected the problem and opened a new issue to track the problem in logs: #2633.

@Pangoraw Pangoraw merged commit 137420c into fonsp:main Aug 27, 2023
@fonsp fonsp added bug Something isn't working backend Concerning the julia server and runtime display & PlutoRunner & AbstractPlutoDingetjes.jl labels Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Concerning the julia server and runtime bug Something isn't working display & PlutoRunner & AbstractPlutoDingetjes.jl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Click "more" for embeddable element doesn't work
4 participants