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

Enhancement of Vega-Embed CSS for Improved Display and Flexibility #2867

Merged
merged 9 commits into from
Apr 26, 2023

Conversation

nlafleur
Copy link
Contributor

@nlafleur nlafleur commented Feb 3, 2023

This pull request includes updates to the CSS for the vega-embed component.
I have set the width to 100% and changed the vega-embed display to flex for better responsiveness.
I also made changes to the position of the details and summary elements to overrule the default vega-embed styling to make sure the action-icon and popup are placed correctly.

@davidgilbertson
Copy link

I think since the vis id is configurable, it shouldn't be hardcoded in the CSS (as #vis). It can probably be removed, or replaced with {{ output_div }} if the extra specificity was required.

@mattijn
Copy link
Contributor

mattijn commented Feb 5, 2023

Thanks for this PR @nlafleur! It's great that there is progress on responsive charts. I'll try to find time this week to test this more thoroughly.
Good point @davidgilbertson! Thanks for raising.

I think this logic can be extended to the universal and inline template as well, meaning that width="container" would also be supported in notebook cells. Can it?!

@mattijn
Copy link
Contributor

mattijn commented Feb 8, 2023

Let me try to break this down.

Within Vega-Lite it is possible to define a fixed width and a container width. Altair uses Vega-Embed for rendering and if we like to support both the fixed width and container width in Altair then there are some changes required to the existing Vega-Embed css by overruling them.
The question is if these changes can be safely overruled or should they be made in Vega-Embed itself. What do you think @domoritz?

The breakdown is made by the following specification,

  1. Chart width="container" (see gist) and
  2. Chart width=200 (see gist)
import altair as alt
from vega_datasets import data

source = data.cars.url

alt.Chart(source, width='container').mark_circle(size=60).encode(
    x='Horsepower:Q',
    y='Miles_per_Gallon:Q',
    color='Origin:N',
).save('chart_container.html')

alt.Chart(source, width=200).mark_circle(size=60).encode(
    x='Horsepower:Q',
    y='Miles_per_Gallon:Q',
    color='Origin:N',
).save('chart_fixed.html')

Without changes to the .vega-embed CSS (before this PR) the charts will look like this:

(1) Chart width="container"
image

No width defined on parent, so chart width becomes 0.

(2) Chart width=200
image

Renders fine. Action button placed correctly


Defining .vega-embed to a width of 100%:

    .vega-embed {
      width: 100%;
    }

Results in:

(1) Chart width="container"
image

Responsive to the full width, but the open dialog of the action menu is to close to the right browser edge.

(2) Chart width=200
image

Chart renders fine on left-side, but the action button is placed on the right-side.


Overruling the following default css from Vega-embed (this PR):

    position: absolute;
    top: 0;
    right: 0;

With:

    .vega-embed {
      width: 100%;
      display: flex;
    }

    .vega-embed details,
    .vega-embed details summary {
      position: relative;
    }

Results in:

(1) Chart width="container"
image

Responsive width and action button on the right side, where the dialog is opened with a buffer on the right side.

(2) Chart width=200
image

Renders fine. Action button placed correctly

@mattijn
Copy link
Contributor

mattijn commented Feb 18, 2023

Gentle ping @domoritz, is this better suited for Vega-embed? Otherwise I will continue here.

@domoritz
Copy link
Member

Thanks for the ping. I am super wary of css changes to embed since there are so many users. I'd need a few hours to test this. Maybe someone could make a pull request with a release in a private package that we can test in jupyter, Streamlit, observable, etc.

@mattijn
Copy link
Contributor

mattijn commented Feb 18, 2023

You can test with this one: https://www.npmjs.com/package/vega-embed-mattijn, https://cdn.jsdelivr.net/npm/vega-embed-mattijn@6. Will do a PR to Vega-embed then.

@mattijn
Copy link
Contributor

mattijn commented Apr 24, 2023

The outstanding PR within vega-embed vega/vega-embed#1066, will lead to changing behavior of existing usages of vega-embed where one overrule existing CSS settings (eg streamlit, but then there will be more). While I think the suggested change will improve vega-embed in general, maybe it is better to opt first to include this within Altair only.

Do more people have an opinion on this?

@binste
Copy link
Contributor

binste commented Apr 24, 2023

I agree that making this change in Altair would be less disruptive as vega-embed is certainly used much broader than the HTML functionality in Altair. I think your findings on Streamlit support this. Also, version 5 would be a good moment to do this so I'd be in favour of merging this (with a changelog entry).

This PR does not affect the HTML mimetype renderer and therefore width="container" charts still do not show up in IDEs such as JupyterLab and VS Code. I assume this is intended and it looks correct to me but just wanted to double-check with you.

@mattijn
Copy link
Contributor

mattijn commented Apr 24, 2023

Good. No, I think I will update this PR to have this responsive behavior also for Altair charts in notebook cells and not in saved html files only.

@joelostblom
Copy link
Contributor

Do more people have an opinion on this?

I don't have strong opinions on this, but if we like the behavior and the only hesitation is which repo the change should go in, then maybe it make sense to merge it here for now as it can always be removed later if the same functionality is merged in vega-embed?

@mattijn
Copy link
Contributor

mattijn commented Apr 25, 2023

I've updated this PR so the response width support is enabled for the HTML_TEMPLATE and INLINE_HTML_TEMPLATE. I was not able to get this to fully work for the HTML_TEMPLATE_UNIVERSAL so I decided to leave that out.

It is hard to inject CSS that works both in the Jupyter ecosystem and MS VSCode where the responsive width is both respecting resizing of the browser (I can only get this to work in Jupyter) and resizing when using panels (not succeeding).

@mattijn
Copy link
Contributor

mattijn commented Apr 25, 2023

Meaning that this comment from @binste applies:

This PR does not affect the HTML mimetype renderer and therefore width="container" charts still do not show up in IDEs such as JupyterLab and VS Code. I assume this is intended and it looks correct to me but just wanted to double-check with you.

@mattijn
Copy link
Contributor

mattijn commented Apr 25, 2023

Hmpf, what I tried was not to include a <style> tag with CSS within the <body>, as apparently is bad-practice, https://stackoverflow.com/questions/2830296/using-style-tags-in-the-body-with-other-html.

But if I just add it like this:

HTML_TEMPLATE_UNIVERSAL = jinja2.Template(
    """
<style>
  #{{ output_div }}.vega-embed {
    width: 100%;
    display: flex;
  }

  #{{ output_div }}.vega-embed details,
  #{{ output_div }}.vega-embed details summary {
    position: relative;
  }
</style>
...

Then it works as expected in MS VScode (both resizing app and panel), and in Jupyter Notebook and upon resizing of the browser in Jupyter Lab
See the following animated gifs:
vscode
notebook
lab

I'm somehow tempted to include this..

Edit, Colab is working as well:
colab

@mattijn
Copy link
Contributor

mattijn commented Apr 25, 2023

Just did that. Please review @binste and @joelostblom🙈

Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

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

I'm not aware of any drawbacks of this solution but I'm also no CSS expert. Only added a minor comment but else it looks good to me to at least try it in the rc2 and see if we spot any issues in other frontends.

doc/releases/changes.rst Outdated Show resolved Hide resolved
@mattijn
Copy link
Contributor

mattijn commented Apr 26, 2023

Thanks for approval @binste! Trial in rc2 sounds good to me too.

@mattijn mattijn merged commit ba4a7d3 into vega:master Apr 26, 2023
@mattijn
Copy link
Contributor

mattijn commented Apr 26, 2023

Thank you for this PR @nlafleur! Appreciated!

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.

6 participants