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 responsiveness container #1066

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mattijn
Copy link

@mattijn mattijn commented Feb 18, 2023

This PR can be tested using the following npm-test package

Further section is copied from: vega/altair#2867


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
Author

mattijn commented Apr 17, 2023

Any good suggestions how I or others can support the review process?

@mattijn
Copy link
Author

mattijn commented Apr 18, 2023

Let me add some comments while trying to assist in the review process.

I've tested it a bit with standard usage of vega-embed. Vega-embed works as usual with this PR when there is no custom overriding of the vega-embed css defaults.

But I noticed that some websites use custom css to override the vega-embed css defaults. One of these is streamlit.

Using tampermonkey and the following script I can see the effects of this PR at a particular website.

// ==UserScript==
// @name         New Userscript
// @namespace    http://tampermonkey.net/
// @version      0.1
// @description  Changes Vega charts' background color and style in Streamlit docs.
// @author       You
// @match        https://doc-vega-lite-chart.streamlit.app/~/+/?embed=true
// @icon         https://www.google.com/s2/favicons?sz=64&domain=streamlit.io
// @grant        none
// ==/UserScript==
(function() {
  'use strict';

  const observer = new MutationObserver((mutationsList, observer) => {
    const vegaElements = document.querySelectorAll('.vega-embed');

    if (vegaElements.length > 0) {
      observer.disconnect();

      vegaElements.forEach(vegaElement => {
        vegaElement.style.display = "flex";

        const detailObserver = new MutationObserver((mutationsList, observer) => {
          const detailsNode = vegaElement.querySelector('details');
          const summaryNode = vegaElement.querySelector('summary');

          if (detailsNode && summaryNode) {
            observer.disconnect();

            detailsNode.style.position = "relative";

            summaryNode.style.position = "unset";
            summaryNode.style.top = "unset";
            summaryNode.style.right = "unset";
          }

        })
        detailObserver.observe(document.body, {
          subtree: true,
          childList: true
        });
      });
    }
  });

  observer.observe(document.body, {
    subtree: true,
    childList: true
  });
})();

The script above only @match with https://doc-vega-lite-chart.streamlit.app/~/+/?embed=true

It seems there is defined a <button /> added outside the <div data-testid="stArrowVegaLiteChart" /> with an absolute position top right, therefor it is above the details.

If you know more sites that use vega-embed including usage of the actions, than I can test these as well.
If you think that this is proof enough already that this PR is too risky to be merged, fine as well.

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.

1 participant