Bugfix: Add proper padding when scrolling is added to bar gauge #32411

mckn · 2021-03-29T09:53:34Z

What this PR does / why we need it:
Prior to this PR the scrollbar will hide the values in the bar gauge (and probably other visualizations). I added a setScrollable to apply the scrolling in the PanelChrome instead of in the vizRepeater which results in getting the padding in the right place for all browsers.

Which issue(s) this PR fixes:
Fixes #32083

Special notes for your reviewer:
I'm a bit concerned about adding the overflow: hidden to the .panel-content. Have done some testing and think it should be fine but still changing that component feels a bit dangerous.

…ion.

packages/grafana-ui/src/components/VizRepeater/VizRepeater.tsx

mckn · 2021-03-29T09:55:36Z

public/sass/pages/_dashboard.scss

@@ -102,11 +102,22 @@ div.flot-text {
  width: 100%;
  flex-grow: 1;
  height: calc(100% - #{$panel-header-height});
+  overflow: hidden;


Note: this feels a bit dangerous. Do you know about any use cases that might introduce a bug by adding this code? @torkelo

it breaks the dropdown select in Table panel, if we had a proper portal for Select component that could solve that

hmm, damn...so I need to rethink this a bit.

@torkelo I changed so we use the portal functionality of react-select...not sure if this is the best way to proceed but I didn't need to do so much work to get it in place.

Let me know if you think this is something we should proceed with. I will wait with fixing the tests until I got a 👍🏻

And I guess we want to support horizontal scrolling as well.

mckn · 2021-03-29T09:56:02Z

public/sass/pages/_dashboard.scss

+    margin-bottom: $panel-padding;
+  }
+
+  &--no-padding.panel-content--scrollable-y {


Note: can I write this in some better way using sass?

Suggested change

&--no-padding.panel-content--scrollable-y {

&--no-padding#{&}--scrollable-y {

might do the trick :)

mckn · 2021-03-31T08:55:43Z

Summary:
When I changed the .panel-content to have overflow: hidden the selector stopped working e.g. in the scenario below. I fixed that by changing so we render the select menu in a portal.

However that resulted in the following issue in scenarios where we have a selector in a modal. That was solved by increasing the zIndex of the select menu portal to same as the modal.

The benefit of doing this is that we also squash a bunch of bugs where we use the selector in e.g. transformations etc where we currently have forced the selector to open downwards to prevent it from being hidden. But again, increasing the zIndex feels like a slippery slope.

I can think of one scenario (not sure if it is a valid one) where having the same (or higher) zIndex of the select menu portal and the modal is if we want to have an open select but still present a modal on top of it.

Would love some feedback on this @grafana/grafana-frontend-platform 🙏🏻

Edit
One thing that would make this a bit more flexible is that you can pass a property that indicates the context to the selector.

Edit 2
Not sure the @grafana/grafana-frontend-platform seems to work. So I will add @jackw @dprokop @hugohaggmark @torkelo @oscarkilhed ...

torkelo · 2021-04-06T14:29:15Z

public/app/features/dashboard/dashgrid/PanelChrome.tsx

+    const scrollableYMargin = plugin.scrollableY ? theme.panelPadding : 0;
+    const innerPanelHeight = height - headerHeight - chromePadding * 2 - PANEL_BORDER - scrollableYMargin;
    const panelContentClassNames = classNames({
      'panel-content': true,
      'panel-content--no-padding': plugin.noPadding,
+      'panel-content--scrollable-y': plugin.scrollableY,


Not sure we need this special logic. and If plugin.scrollable is true we should wrap the component in a CustomScrollbar

…to mckn/bar-gauge-scroll

…nges

dprokop · 2021-04-08T07:25:55Z

@mckn this fixes the bar gauge issue 👌 But I see that adding portaling to select introduces the exact issues me and @tskarhed has been struggling with when we tried to add portaling:

The panel edit selects are now positioned incorrectly, add scrollbar to the viewport and have fixed position relative to the window when scrolling. This applies to query editor selects and other selects as well
The initial position of the select is incorrect:

tskarhed · 2021-04-08T07:45:59Z

Wohoo! Select issues, don't you just love them? 😁

mckn · 2021-04-08T08:04:16Z

@mckn this fixes the bar gauge issue 👌 But I see that adding portaling to select introduces the exact issues me and @tskarhed has been struggling with when we tried to add portaling:

There's sth wrong with Table's panel select:

The panel edit selects are now positioned incorrectly, add scrollbar to the viewport and have fixed position relative to the window when scrolling. This applies to query editor selects and other selects as well

The initial position of the select is incorrect:

Awesome findings @dprokop! I think for some of this issues we probably need to expose a "usePortal" property on the select component so you can control that depending on the context. It doesn't feel good but it might be the way to go.

mckn · 2021-04-09T09:37:17Z

Closing this one in favor of #32835 and #32833

fixing so we get the scrollbar in the proper place for the vizualizat…

5e9e047

…ion.

mckn added type/bug area/panel/gauge add to changelog old backport v7.5.x Mark PR for automatic backport to v7.5.x labels Mar 29, 2021

mckn added this to the 7.5.2 milestone Mar 29, 2021

mckn requested review from torkelo and a team March 29, 2021 09:53

mckn added this to In Review in Frontend Platform Backlog via automation Mar 29, 2021

mckn self-assigned this Mar 29, 2021

mckn requested review from peterholmberg and removed request for a team March 29, 2021 09:53

grafanabot added the area/frontend label Mar 29, 2021

mckn commented Mar 29, 2021

View reviewed changes

packages/grafana-ui/src/components/VizRepeater/VizRepeater.tsx Show resolved Hide resolved

mckn commented Mar 29, 2021

View reviewed changes

changed so the select is used with a portal.

6df7ac7

mckn modified the milestones: 7.5.2, 7.5.3 Mar 29, 2021

mckn requested a review from dprokop March 30, 2021 13:51

increasing the zIndex of the selector.

5a7aa39

torkelo reviewed Apr 6, 2021

View reviewed changes

torkelo added 5 commits April 6, 2021 16:48

Merge remote-tracking branch 'origin/master' into mckn/bar-gauge-scroll

f493ec8

Testing custom scrollbar

1746025

Merge branch 'mckn/bar-gauge-scroll' of github.com:grafana/grafana in…

380eda8

…to mckn/bar-gauge-scroll

Progress but now DashList has no right padding

54c3681

added back className

faed5ca

ifrost modified the milestones: 7.5.3, 7.5.4 Apr 7, 2021

mckn and others added 4 commits April 7, 2021 15:38

wip

aaf8e39

fixing the issues with zIndexes and having the select as a portal.

24d9eef

adding portal to theme definition.

ea3ac70

test(selectbase): migrate tests to RTL and fix for portal related cha…

d7810fc

…nges

mckn mentioned this pull request Apr 9, 2021

Select: changing so we render the menu in a portal to fix issues with menu being hidden #32833

Closed

2 tasks

mckn closed this Apr 9, 2021

Frontend Platform Backlog automation moved this from In Review to Done Apr 9, 2021

osg-grafana changed the title ~~Bugfix: making sure we have proper padding when scrolling is added to bar gauge~~ Bugfix: Make sure we have proper padding when scrolling is added to bar gauge Apr 13, 2021

osg-grafana changed the title ~~Bugfix: Make sure we have proper padding when scrolling is added to bar gauge~~ Bugfix: Add proper padding when scrolling is added to bar gauge Apr 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: Add proper padding when scrolling is added to bar gauge #32411

Bugfix: Add proper padding when scrolling is added to bar gauge #32411

mckn commented Mar 29, 2021

mckn Mar 29, 2021

torkelo Mar 29, 2021

mckn Mar 29, 2021

mckn Mar 29, 2021

mckn Mar 30, 2021

mckn Mar 30, 2021

mckn Mar 29, 2021

Elfo404 Apr 5, 2021

mckn commented Mar 31, 2021 •

edited

torkelo Apr 6, 2021

dprokop commented Apr 8, 2021 •

edited

tskarhed commented Apr 8, 2021

mckn commented Apr 8, 2021

mckn commented Apr 9, 2021

	&--no-padding.panel-content--scrollable-y {
	&--no-padding#{&}--scrollable-y {

Bugfix: Add proper padding when scrolling is added to bar gauge #32411

Bugfix: Add proper padding when scrolling is added to bar gauge #32411

Conversation

mckn commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mckn commented Mar 31, 2021 • edited

Choose a reason for hiding this comment

dprokop commented Apr 8, 2021 • edited

tskarhed commented Apr 8, 2021

mckn commented Apr 8, 2021

mckn commented Apr 9, 2021

mckn commented Mar 31, 2021 •

edited

dprokop commented Apr 8, 2021 •

edited