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

Improve experience with HTML rendering of dataframes #300

Merged
merged 8 commits into from
Mar 31, 2023
Merged

Conversation

koperagen
Copy link
Collaborator

In this PR i'll to address issues that exist with our HTML rendering

  1. It wasn't possible to render DataFrame so that it can be rendered in the browser (initHtml is an internal function, and it's not clear what it does)
  2. We lack documentation for custom-rendering-in-Jupyter use-case
  3. It exposes HtmlData class that is not a part of our API
  4. ...?

*/
public fun <T> DataFrame<T>.toStandaloneHTML(
configuration: DisplayConfiguration = DisplayConfiguration.DEFAULT,
cellRenderer: CellRenderer = org.jetbrains.kotlinx.dataframe.jupyter.DefaultCellRenderer,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I think our idea was to move everything Jupyter into a separate module, right? I think we should have a Jupyter-less toHTML method :)

Copy link
Collaborator Author

@koperagen koperagen Mar 15, 2023

Choose a reason for hiding this comment

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

Thing is, this is our class :) I was confused too. The idea was to move Integration to another module, so we cannot accidentally use something that is not available outside notebooks. I think this one is ok

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh hmm, odd XD. Then it could be fine for now I think. As long as we take up #241 in the future :)

@pacher
Copy link
Contributor

pacher commented Mar 15, 2023

Can we please, please just make extraHtml parameter of toHtml() default to initHtml() instead of null. Please?

Before 8e596e6 it was just a boolean named includeInit and everything was fine.
Now it is called extraHtml and defaults to null which is causing many problems for no apparent reason.

  • toHtml() by default does not include javascript and css because extraHtml is null, so the output is not very usable. And I can not just do toHtml(extraHtml = initHtml()) because initHtml() is internal. So I had to copy-paste and adapt initHtml where I load resources from the dataframe dependency (!). And now I depend on implementation detail and have to monitor every commit in dataframe just in case something has changed.
  • there is a format operator which produces FormattedFrame and the only thing one can do with it is to render it to html using FormattedFrame.toHTML(). But as you might have already guessed, it is calling DataFrame.toHtml() without extraHtml = initHtml() and the output is unusable due to the lack of javascript and css. Clearly it was overlooked after change in 8e596e6

And the thing is, I can't think of a use case where extraHtml should be null, let alone by default. The only place I have found in the dataframe codebase where it is used with default null is in some tests where it does not really matter, just saves a few bytes of memory and a few cpu cycles.
I do believe that if such a use case exists, it is justifiable to explicitly pass null as a parameter, like for example in those tests.

P.S. How can I make code suggestions? Should I make a PR against this PR branch or something?

@koperagen
Copy link
Collaborator Author

Hi! Sorry for the inconvenience, it was indeed overlooked. With this PR toStandaloneHTML is toHTML with default to initHtml. I think it's what you want, no? I want to keep toHTML with null becuase at least on Kotlin Notebooks our script for tables is provided by the environment, so no need to include it. I.e.
Use df.toStandaloneHTML() to save this HTML as a page
Use DISPLAY(HTML(df.toHTML())) in notebooks to render HTML with custom DisplayConfiguration or formatting. Or am i wrong and this output doesn't work in notebooks?

@pacher
Copy link
Contributor

pacher commented Mar 15, 2023

@koperagen Thanks for quick reply.
I did not use notebooks yet, so can't say what works there. So I did not know about DISPLAY(HTML(df.toHTML())), unfortunately for me it looks like a valid use case.

Yes, I've seen that toStandaloneHTML() includes initHtml and that's why my thumbs up.
I would still use toHtml because it returns HtmlData where it is easy to add my own bits of javascript and css. Which brings us to

3. It exposes HtmlData class that is not a part of our API

I've started to use HtmlData when it still was a part of dataframe public api before it was removed in 3db46cc
Now it still works, but I need to declare org.jetbrains.kotlinx:kotlin-jupyter-api:0.11.0-198 dependency and again not to forget to watch every commit in dataframe for a version change. Would be nice to have it as api level dependency of dataframe, but I guess it's not going to happen.

Back to the topic, can we maybe than rename parameter extraHtml to initHtml and make initHtml() function public with some other more descriptive name? Like loadInitHtmlFromResources or something..

@koperagen
Copy link
Collaborator Author

koperagen commented Mar 15, 2023

Oh, okay, thank you for your perspective. I wasn't sure if we should create our own public version of HtmlData, but now i see that we should. For external use, it looks like plus overload and toString is enough, right?
Unfortunately can't make org.jetbrains.kotlinx:kotlin-jupyter-api an api dependency, it's only a compileOnly because Jupyter kernel provides these classes.

@pacher
Copy link
Contributor

pacher commented Mar 15, 2023

For external use, it looks like plus overload and toString is enough, right?

Right, that's the only things dataframe's HtmlData had and the only ones I use.

Oh, okay, thank you for your perspective. I wasn't sure if we should create our own public version of HtmlData, but now i see that we should.

Now I feel bad if public version will be created only because of me. Maybe what I do is too much special and could/should be done otherwise. I just do a little tweaking, for example I want to align everything right, not just numbers, so I have

private fun HtmlData.alignAllRight() = this + HtmlData("""
    table.dataframe th, td {
        text-align: right;
    }
""".trimIndent(), "", "")

but that is bad example, because it could be done with format
Another example is that I want all column groups be expanded by default, so I have

private fun HtmlData.addExpandAllColumnsFunction() = this + HtmlData("", "", """
    function expandAllColumns() {
        const tableRootId = document.getElementsByClassName('dataframe')[0].df.rootId;
        const dframe = document.getElementById("df_" + tableRootId).df;
        dframe.cols.forEach((col) => {if (col.children.length > 0) col.expanded = true});
        for (frameId in dframe.childFrames) dframe.childFrames[frameId].cols.forEach((col) => {if (col.children.length > 0) col.expanded = true});
        DataFrame.renderTable(tableRootId);
    }                    
""".trimIndent())

private fun HtmlData.expandAllColumns() = addExpandAllColumnsFunction() + HtmlData("", "", """
    expandAllColumns();
""".trimIndent())

and similar to expand all nested frames.
This kind of stuff. (I know that I depend on javascript in resources, that's on me)
There are probably too many of such possible little tweaks to try and have configuration options in toHtml().

At the end I display it in JavaFx WebView, so I can even interact with it if needed and call javascript functions from kotlin.

@koperagen
Copy link
Collaborator Author

koperagen commented Mar 18, 2023

Hi @pacher i published this draft as a dev version 0.10.0-dev-1456, check it out if you want c:
I think it is a good idea to have such API for customizing resulting HTML, because we indeed cannot cover all possible tweaks. I've also added a couple of short cut functions that might prove useful when working with HtmlData: writeHTML and openInBrowser

@pacher
Copy link
Contributor

pacher commented Mar 23, 2023

@koperagen
Awesome! Thanks a lot.

Couple of remarks:

  1. From what I can see javadocs got swapped for toHtml and toStandaloneHTML in FormattedFrame. Based on implementation toHtml should be without definitions and toStandaloneHTML should be with definitions.
  2. If we are targeting modern kotlin and JDK, maybe we should add an overload with java.nio.file.Path to writeHTML(destination: File)?
  3. Maybe you can have a look and close PR Remove unused useDarkColorScheme parameter from DisplayConfiguration #236. Accept or reject it does not matter. It's not important, but would make merge conflicts or just hang there forever.

@koperagen
Copy link
Collaborator Author

@koperagen Awesome! Thanks a lot.

Couple of remarks:

  1. From what I can see javadocs got swapped for toHtml and toStandaloneHTML in FormattedFrame. Based on implementation toHtml should be without definitions and toStandaloneHTML should be with definitions.
  2. If we are targeting modern kotlin and JDK, maybe we should add an overload with java.nio.file.Path to writeHTML(destination: File)?
  3. Maybe you can have a look and close PR Remove unused useDarkColorScheme parameter from DisplayConfiguration #236. Accept or reject it does not matter. It's not important, but would make merge conflicts or just hang there forever.

Thanks! I checked that Path has some advantages over File even in such a simple case, so i'll add it.

Path claims it has better platform independence, unicode support and better performance, so people might have reason to choose it over File
@koperagen koperagen marked this pull request as ready for review March 29, 2023 16:37
docs/StardustDocs/topics/toHTML.md Outdated Show resolved Hide resolved
docs/StardustDocs/topics/toHTML.md Outdated Show resolved Hide resolved

### Configuring display for individual output

`toHTML` is useful if you want to configure display for single cell, not whole notebook as described in section for [Jupyter Notebooks](jupyterRendering.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

grammar ;P
configure how a single cell is displayed. How to configure this for the entire notebook, check out this section..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, thanks for pointing that out. I rephrased this sentence

@koperagen koperagen merged commit 3861e1d into master Mar 31, 2023
@koperagen koperagen deleted the html-api branch April 12, 2023 10:03
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.

3 participants