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

Lift AbstractString type restriction on summary arg of details #296

Merged
merged 5 commits into from
Mar 4, 2024

Conversation

genericallyterrible
Copy link
Contributor

Fixes: #295

Allows any type to be used as summary and leaves it up to the user to decide what is rational.

Unlike the details argument, summary is not embedded using embed_display. Instead summary is only wrapped by a simple div with appropriate CSS to provide a decent display for html-like objects.

Examples

summary type Output
String (unchanged) image
@md image
@htl image
other (dict) image

Copy link

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="non-str-summary")
julia> using PlutoUI

Or run this code in your browser: Run with binder

@genericallyterrible
Copy link
Contributor Author

Chrome is currently unhappy :(

image

@genericallyterrible
Copy link
Contributor Author

Parity on my machine.

Browser Example
Chrome image
Firefox image

@genericallyterrible
Copy link
Contributor Author

Quoting: @rgouveiamendes from #290 (comment)

... Is there any particular reason why you prefer display: inline-block; to display: inline-flex;? The difference I see is the position of the triangle at the top or bottom of the block. ...

No particular conviction, inline-block was the only option I saw when looking for examples of elements nested within a <summary> tag and it looked good to me.

Upon comparison, inline-flex is most similar to default <summary> behavior with simple contents, though I don't think any nested div solution could perfectly mimic default behavior.

Style Result
Default <summary>, no nested <div> image
Nested div with display: inline-block; (current) image
Nested div with display: inline-flex; (proposed) image

While doing this comparison I have become partial to inline-flex, but unfortunately it has its own problems! Yet somehow inline-table appears to be a potential panacea to our present predicament.

Style Result
Nested div with display: inline-flex; image
Nested div with display: inline-table; image

I cannot say I fully comprehend the depth of the implications were we to opt for inline-table. inline-block makes simple sense to my grug brain, but I'll gladly change the display if those with bigger brains say unto me, "grug, inline-table is good."

grug

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.

Wow another awesome PR! Thanks so much!

I like the style of inline-flex more with the wrapping markdown title, but if you say it has problems then I'm happy with inline-block! I prefer to not use a table styles outside of tables because they give me bad vibes but for this case it would also be okay!

It's just a very weird element to style because the <summary> is implemented by the UA as a list item with the > as list marker... If you found something that looks good and consistent on all browsers then I'm happy!

@fonsp
Copy link
Member

fonsp commented Feb 29, 2024

I'm really happy with your contributions to PlutoUI! Are you interested in other projects? I'm thinking about:

Do you know JavaScript? In that case there are lots of possible projects similar to https://github.com/lukavdplas/PlutoMapPicker.jl . Also check out the recent new feature: fonsp/Pluto.jl#2726

Anyway, big thanks for your work so far and maybe we can find something fun to work on! I'm also happy to chat on zulip or have a video call to discuss!

@rgouveiamendes
Copy link

Wow! @fonsp let me see what I can do. I will analyze the links you've mentioned. However, I am quite busy right now... :( Maybe just in summer.

Regarding flex-inline what are exactly the problems you detected @genericallyterrible? I am using it without any problem in Brave (Chrome).

@genericallyterrible
Copy link
Contributor Author

Regarding flex-inline what are exactly the problems you detected @genericallyterrible?

My screenshots above could've been clearer and I should've explicitly stated what I found rather than making assumptions, apologies. With inline-flex, summaries made with @md are fantastic and appear exactly as I'd expect but summaries made with @htl aren't nearly as pretty :(

The following screenshot showcases functionally equivalent markdown and html producing wildly different results when using display: inline: flex.
image

inline-table is not consistent across Chrome and Firefox so there goes that idea 🙃

Nothing an extra wrapper can't fix though, eh?
image

@fonsp
Copy link
Member

fonsp commented Mar 1, 2024

Looks good! Let's merge?

@rgouveiamendes My invitation for future contributions was meant more for @genericallyterrible who made a couple of really well implemented pull requests recently but of course everyone should feel welcome to contribute! Open a draft PR when you start working and let's make something nice!

@genericallyterrible
Copy link
Contributor Author

I'm ready to send it!

@fonsp fonsp merged commit b6366bd into JuliaPluto:main Mar 4, 2024
3 checks passed
@genericallyterrible genericallyterrible deleted the non-str-summary branch March 5, 2024 21:45
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.

Lift AbstractString type restriction on summary arg of details
3 participants