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

Allow objects with a _repr_html_() method to be rendered to HTML #74

Merged
merged 5 commits into from
Nov 15, 2023

Conversation

cpsievert
Copy link
Collaborator

@cpsievert cpsievert commented Nov 14, 2023

As we've pointed out in several py-shiny/py-shinywidget issues (e.g., posit-dev/py-shiny#303), rendering "ad hoc (i.e., non-ipywidget) widgets" like folium or itables is possible, but it isn't easy since to do because you need ui.HTML(x._repr_html_()) to get the relevant HTML.

This PR makes it so objects that implement a _repr_html_ method are renderable when they appear as a child of a Tag/TagList. Also note that, using this approach, we can statically and dynamically render these objects:

import folium

from shiny import App, render, ui

app_ui = ui.page_fixed(
    "Map 1",
    folium.Map(location=(45.5236, -122.6750)),
    ui.br(),
    "Map 2",
    ui.output_ui("map")
)

def server(input, output, session):
    @output
    @render.ui
    def map():
        return folium.Map(location=(45.5236, -122.6750))

app = App(app_ui, server)

@cpsievert cpsievert requested a review from wch November 14, 2023 17:25
@cpsievert cpsievert force-pushed the repr-html branch 3 times, most recently from 51aa2bb to 321b7a2 Compare November 14, 2023 17:34
pyrightconfig.json Outdated Show resolved Hide resolved
Comment on lines 329 to 332
if isinstance(child, ReprHtml):
child = HTML(child._repr_html_()) # type: ignore

# At this point, child must be a string/HTML.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is added inside of the else code block, but I think it would be clearer if it were added at the same level as the else. Something like this makes it really clear what's going on (and the reader doesn't have to look at _normalize_text to find out that it's a no-op for HTML objects):

            elif isinstance(child, ReprHtml):
                if prev_was_add_ws:
                    html_ += "  " * indent

                html_ += HTML(child._repr_html_())  # type: ignore[reportPrivateUsage]

                prev_was_add_ws = False

One more thing I wonder about: the code as currently written assumes that the _repr_html_ method does not want to have space around it. But in practice, I think that objects with _repr_html_ are always block elements, because they're meant to be displayed beneath a notebook cell. So perhaps the whitespace logic should be changed to reflect that? Or are there any cases of _repr_html_ where they want to be inline?

Copy link
Collaborator Author

@cpsievert cpsievert Nov 15, 2023

Choose a reason for hiding this comment

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

Done in fb78cda. I agree that most if not all _repr_html_ cases should be block elements, but I don't think it's terrible to say x._repr_html() gets the same treatment as HTML(), especially considering that adding whitespace correctly would add considerable complexity here without obvious gain (we can't control whitespace within x._repr_html(), so producing readable HTML is mostly up to x).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just mean that if we treat it as block element, we'd do this:

            elif isinstance(child, ReprHtml):
                html_ += "  " * indent

                html_ += HTML(child._repr_html_())  # type: ignore[reportPrivateUsage]

                prev_was_add_ws = True

Copy link
Collaborator Author

@cpsievert cpsievert Nov 15, 2023

Choose a reason for hiding this comment

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

I think what you're wanting would be more involved to implement correctly, though. With your suggestion, there is nothing to handle to eol whitespace (here's a minimal example):

import htmltools as h

class Foo:
    def _repr_html_(self) -> str:
        return "<span>Foo</span>"

print(str(h.span(f)))
<span>  <span>Foo</span></span>

As I said before, it doesn't feel worth the added complexity for such a minor detail. Plus, I like that the behavior is consistent with HTML()

Copy link
Collaborator

Choose a reason for hiding this comment

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

The behavior in your example (using the code I suggested) is the same as the current behavior if you put a block (_add_ws=True) element inside of an inline (_add_ws=False) element.

import htmltools as h
print(str(h.span(h.div())))
<span>  <div></div></span>

Do you think we should expect some different behavior here?

Copy link
Collaborator Author

@cpsievert cpsievert Nov 15, 2023

Choose a reason for hiding this comment

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

Oh yea, I do find that surprising, but it doesn't seem like a big problem since that's not valid HTML

@cpsievert cpsievert marked this pull request as ready for review November 15, 2023 16:21
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