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

Add details #288

Merged
merged 15 commits into from
Feb 15, 2024
Merged

Add details #288

merged 15 commits into from
Feb 15, 2024

Conversation

genericallyterrible
Copy link
Contributor

@genericallyterrible genericallyterrible commented Feb 8, 2024

Adds new function details to automatically create a well formed Details disclosure element (<details>).

Currently scoped as:

using PlutoUI.ExperimentalLayout: details

using PlutoUI #: details

Example usage of new details function

Since this new function creates normal HTML <details> elements, collapsed details elements will omit their contents from any print/export. If this behavior is undesirable, it seems a JS fix could be applied as discussed here on SO though I'm unsure where it would slot in.

I couldn't get @bind to behave properly in the Layout.jl notebook to fully test it internally. UI elements would render, but bound variables would never update. When using details in the binder environment automatically created by github-actions it behaves as expected.

Example usage of new details function with bound variables

Copy link

github-actions bot commented Feb 8, 2024

Try this Pull Request!

Open Julia and type:

julia> import Pkg
julia> Pkg.activate(temp=true)
julia> Pkg.add(url="https://github.com/genericallyterrible/PlutoUI.jl", rev="summary-details")
julia> using PlutoUI

Or run this code in your browser: Run with binder

src/Layout.jl Outdated Show resolved Hide resolved
@fonsp
Copy link
Member

fonsp commented Feb 8, 2024

Super nice!! And really well implemented, happy to see that you are using HypertextLiteral!! I would say that this is not "experimental", and it can go directly into PlutoUI. What do you think?

@genericallyterrible
Copy link
Contributor Author

genericallyterrible commented Feb 9, 2024

Sounds good to me! Wasn't super sure where details should end up, and honestly stumbled my way into Layout.jl. If it should live in some other file so it's not submoduled under ExperimentalLayout, the only reliance it has on the other code in Layout.jl is the 5 lines implementing embed_display (and the variable p for example purposes).

I can move details into its own notebook and copy over the functionality of embed_display if that's appropriate, or perhaps there's some other path you had in mind?

Update per review

Co-authored-by: Fons van der Plas <fonsvdplas@gmail.com>
@genericallyterrible
Copy link
Contributor Author

genericallyterrible commented Feb 9, 2024

details has been moved to Details.jl and is now reexported from PlutoUI.jl

Edit: Running on the binder image:

Demonstration of this pull request running on binder

Copy link
Member

@fonsp fonsp left a comment

Choose a reason for hiding this comment

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

Hey! Again this PR is really impressive! Thanks so much for the contribution :)

After this PR is merged, I would like to move all of the CSS into Pluto itself: a details element created without PlutoUI should also get these nice styles :) Can you make a PR to Pluto for that?

I made some small tweaks, see the comments. To me this is ready to merge, let me know!

src/Details.jl Outdated

# ╔═╡ 46521e2b-ea06-491a-9842-13dff7dc8299
begin
const embed_detail = isdefined(Main, :PlutoRunner) && isdefined(Main.PlutoRunner, :embed_display) ? Main.PlutoRunner.embed_display : identity
Copy link
Member

Choose a reason for hiding this comment

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

FYI this can later be replaced with JuliaPluto/AbstractPlutoDingetjes.jl#9

src/Details.jl Outdated Show resolved Hide resolved
@genericallyterrible
Copy link
Contributor Author

Thanks so much for the tweaks and tests!

I'd be happy to PR the CSS over to Pluto once this is accepted, everyone deserves pretty <details>. 😊

I have a bit of clarification on one of your changes and its impacts on the assumptions I made about how details should format its contents, but I'll leave that comment as a response to the change itself.

@fonsp fonsp merged commit a8889b5 into JuliaPluto:main Feb 15, 2024
3 checks passed
@fonsp
Copy link
Member

fonsp commented Feb 15, 2024

Thanks so much for the PR! Let me know what you would like to work on next :)

genericallyterrible added a commit to genericallyterrible/Pluto.jl that referenced this pull request Feb 15, 2024
genericallyterrible added a commit to genericallyterrible/PlutoUI.jl that referenced this pull request Feb 15, 2024
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