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

[Vega] Fix vega controls layout #130954

Merged
merged 5 commits into from
Apr 28, 2022
Merged

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Apr 26, 2022

Fixes #130436 There were multiple problems with resizing/layouting in the presence of controls, I tried to fix them all

Test cases:

  • In Chrome and Firefox (because there were problems with scroll bars in Firefox before)
  • Autosize spec without controls
  • Autosize spec with controls
  • Non-autosize spec without controls (smaller than the available space in the panel)
  • Non-autosize spec without controls (larger than the available space in the panel)
  • Non-autosize spec with controls (smaller than the available space in the panel)
  • Non-autosize spec with controls (larger than the available space in the panel)
  • Different positions of the controls via kibana config object: https://www.elastic.co/guide/en/kibana/current/vega.html#vega-additional-configuration-options

Controls should always be rendered at the specified position in the panel if there is enough space (autosize spec or smaller than available space non-autosize). In case there is not enough space (non-autosize larger than available space), the visualization should push out the controls and the whole thing should be scrollable.

@flash1293 flash1293 added release_note:fix Feature:Vega Vega visualizations Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.3.0 labels Apr 26, 2022
@flash1293 flash1293 marked this pull request as ready for review April 27, 2022 09:43
@flash1293 flash1293 requested review from a team as code owners April 27, 2022 09:43
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Overall this works much better than before because we can achieve the correct state, but there are some problems when switching to 'fit-x'/'fit-y' value. After saving or resizing the window it works fine. Steps to reproduce:

  1. Use this code to create Vega vis:
{
  "$schema": "https://vega.github.io/schema/vega/v5.json",
  "description": "A basic bar chart example, with value labels shown upon mouse hover.",

  "padding": {"bottom": 150},
"autosize": "none",
"height":3000, 
"width":1000,
  "data": [
    {
      "name": "table",
      "values": [
        {"category": "A", "amount": 28},
        {"category": "B", "amount": 55},
        {"category": "C", "amount": 43},
        {"category": "D", "amount": 91},
        {"category": "E", "amount": 81},
        {"category": "F", "amount": 53},
        {"category": "G", "amount": 19},
        {"category": "H", "amount": 87}
      ]
    }
  ],

  "signals": [
   { "name":"mypadding", "value": 0.05, "bind": {"input":"range", "min": 0, "max": 1, "step":0.05} }, 
    {
      "name": "tooltip",
      "value": {},
      "on": [
        {"events": "rect:mouseover", "update": "datum"},
        {"events": "rect:mouseout",  "update": "{}"}
      ]
    }
  ],

  "scales": [
    {
      "name": "xscale",
      "type": "band",
      "domain": {"data": "table", "field": "category"},
      "range": "width",
      "padding": {"signal":"mypadding"},
      "round": true
    },
    {
      "name": "yscale",
      "domain": {"data": "table", "field": "amount"},
      "nice": true,
      "range": "height"
    }
  ],

  "axes": [
    { "orient": "bottom", "scale": "xscale" },
    { "orient": "left", "scale": "yscale" }
  ],

  "marks": [
    {
      "type": "rect",
      "from": {"data":"table"},
      "encode": {
        "enter": {
          "x": {"scale": "xscale", "field": "category"},
          "width": {"scale": "xscale", "band": 1},
          "y": {"scale": "yscale", "field": "amount"},
          "y2": {"scale": "yscale", "value": 0}
        },
        "update": {
          "fill": {"value": "steelblue"}
        },
        "hover": {
          "fill": {"value": "red"}
        }
      }
    },
    {
      "type": "text",
      "encode": {
        "enter": {
          "align": {"value": "center"},
          "baseline": {"value": "bottom"},
          "fill": {"value": "#333"}
        },
        "update": {
          "x": {"scale": "xscale", "signal": "tooltip.category", "band": 0.5},
          "y": {"scale": "yscale", "signal": "tooltip.amount", "offset": -2},
          "text": {"signal": "tooltip.amount"},
          "fillOpacity": [
            {"test": "datum === tooltip", "value": 0},
            {"value": 1}
          ]
        }
      }
    }
  ]
}
  1. Change autosize to fit-x

You will see the scrolls:
Screenshot 2022-04-27 at 13 50 04

Also, switching from fit-x to none doesn't show scrollbar which is maybe even more troubling.

@flash1293
Copy link
Contributor Author

Thanks for the review @mbondyra - this was caused by setting the autoresize class in the wrong place. I put that code into resize, but for autosize: none, it's never called. With this change, I can't reproduce the issue anymore. Could you check again?

Initial render:
Screenshot 2022-04-27 at 16 06 26

After switching to fix-x
Screenshot 2022-04-27 at 16 06 34

After switching back to none
Screenshot 2022-04-27 at 16 06 41

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visTypeVega 1.6MB 1.7MB +1.1KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

LGTM, rechecked on Chrome and FF 🆗

@flash1293 flash1293 added the backport:skip This commit does not require backporting label Apr 28, 2022
@flash1293 flash1293 merged commit 8e09cdc into elastic:main Apr 28, 2022
dmlemeshko pushed a commit to dmlemeshko/kibana that referenced this pull request May 5, 2022
* fix vega controls layout

* fix the last case

* adjust snapshot

* make sure autoresize class is set correctly
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
* fix vega controls layout

* fix the last case

* adjust snapshot

* make sure autoresize class is set correctly
flash1293 added a commit to flash1293/kibana that referenced this pull request Oct 12, 2022
flash1293 added a commit to flash1293/kibana that referenced this pull request Oct 12, 2022
flash1293 added a commit that referenced this pull request Oct 19, 2022
* Revert "[Vega] Fix vega controls layout (#130954)"

This reverts commit 8e09cdc.

* Revert "Revert "[Vega] Fix vega controls layout (#130954)""

This reverts commit cd753c3.

* fix vega problems

* always make the vega view scroll

* restrict maximum height
kc13greiner pushed a commit to kc13greiner/kibana that referenced this pull request Oct 19, 2022
* Revert "[Vega] Fix vega controls layout (elastic#130954)"

This reverts commit 8e09cdc.

* Revert "Revert "[Vega] Fix vega controls layout (elastic#130954)""

This reverts commit cd753c3.

* fix vega problems

* always make the vega view scroll

* restrict maximum height
guskovaue pushed a commit to guskovaue/kibana that referenced this pull request Oct 22, 2022
* Revert "[Vega] Fix vega controls layout (elastic#130954)"

This reverts commit 8e09cdc.

* Revert "Revert "[Vega] Fix vega controls layout (elastic#130954)""

This reverts commit cd753c3.

* fix vega problems

* always make the vega view scroll

* restrict maximum height
e40pud pushed a commit to e40pud/kibana that referenced this pull request Oct 24, 2022
* Revert "[Vega] Fix vega controls layout (elastic#130954)"

This reverts commit 8e09cdc.

* Revert "Revert "[Vega] Fix vega controls layout (elastic#130954)""

This reverts commit cd753c3.

* fix vega problems

* always make the vega view scroll

* restrict maximum height
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Vega Vega visualizations release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Displaying a signal element in Vega visualization within a Kibana dashboard
5 participants