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

Dashboard: Overflow and z-index adjustments #75597

Merged
merged 8 commits into from Oct 6, 2023

Conversation

leeoniya
Copy link
Contributor

@leeoniya leeoniya commented Sep 28, 2023

this extracts some needed adjustments from the tooltips PR: #70872

the main thing we need to verify is that it doesn't break existing stuff. primarily overflow in things that which SplitPane and/or SplitPaneWrapper, such as Explore and Panel Edit.

specifically, this undoes the overflow: hidden style of the external SplitPane component. this allows a tooltip portalled into a panel within the SplitPane to render over the panel edit sidebar or into the adjacent (right) split pane in Explore.

previous/related changes:

#75499
#75382
#75328

@leeoniya leeoniya added this to the 10.2.x milestone Sep 28, 2023
@leeoniya leeoniya requested review from a team September 28, 2023 02:41
@leeoniya leeoniya self-assigned this Sep 28, 2023
@leeoniya leeoniya requested a review from a team as a code owner September 28, 2023 02:41
@leeoniya leeoniya requested review from tskarhed and ashharrison90 and removed request for a team September 28, 2023 02:41
@leeoniya leeoniya marked this pull request as draft September 28, 2023 03:45
Comment on lines 365 to 368
// matches .react-grid-item styles in _dashboard_grid.scss to ensure any contained tooltips occlude adjacent panels
'&:hover, &:active, &:focus': {
zIndex: 999,
},
Copy link
Contributor Author

@leeoniya leeoniya Sep 28, 2023

Choose a reason for hiding this comment

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

this handles the case then the panel is not in a grid, such as via PanelRenderer. logically matches the changes in https://github.com/grafana/grafana/pull/75382/files

Copy link
Member

Choose a reason for hiding this comment

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

🤔 it seems this change conflicts with the ability to resize the panel using drag a drop.

NotPossibleToResize.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, good catch. i wonder what the z-index is of that drag handle 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in b3dcbe8

Comment on lines +361 to +363
'> *': {
zIndex: 0,
},
Copy link
Contributor Author

@leeoniya leeoniya Sep 28, 2023

Choose a reason for hiding this comment

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

this prevents tooltips (even with zIndex of 1040) from going under things like RadioButtonGroup such as those in the Explore graph header, where the options have explicit z-index of 1 or 3. we also have explicit z-index on form Input elements which makes them render over the tooltip when it overlaps an adjacent panel.

here's the problem this solves (this is with the new tooltip approach which is portalled into the panel content or the viz itself instead of the global portal):

image

@@ -141,6 +142,9 @@ const getStyles = (theme: GrafanaTheme2, hasSplit: boolean) => {
`;

return {
splitPane: css`
overflow: visible !important;
Copy link
Contributor Author

@leeoniya leeoniya Sep 28, 2023

Choose a reason for hiding this comment

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

in panel edit, this allows the tooltip to exit the viz SplitPane and render on top of the options sidebar.

i have not found this to have any adverse effects, but could use another pair of 👀

without this change:

image

😢 unfortunately, it doesnt have the desired effect in Explore due to the additional layers of various overflow: hidden/scroll/auto rules from react-custom-scrollbars. this will be solved differently in another PR

image

@leeoniya leeoniya marked this pull request as ready for review September 28, 2023 18:41
@leeoniya leeoniya added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Sep 28, 2023
@Elfo404
Copy link
Member

Elfo404 commented Oct 5, 2023

to me this works also in Explore (Chrome, FF - mac), is #75597 (comment) still relevant?

Screen.Recording.2023-10-05.at.12.17.01.mov

the only "weird" thing is the scrollbar appearing above the tooltip (which doesn't happen in main).

Copy link
Member

@axelavargas axelavargas left a comment

Choose a reason for hiding this comment

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

Hey @leeoniya 👋🏾 ,

I double-checked the issue regarding drag and drop and did additional testing. Besides Gio's comment, I could not find anything else 🏅

P.S I left a minor comment :)

@@ -13,6 +13,8 @@
&:hover {
.react-resizable-handle {
visibility: visible;
// above other panels, but below lowest pre-defined index in zIndex.ts
z-index: 999;
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we would benefit from defining this z-index value in zIndex.ts? It seems that the value is context-dependent, and I am concerned that if that file changes its values, this element will no longer be below the lowest pre-defined index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i wanted to put it there, but how do we import it into scss files? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

🤔 that's a great question, it seems we already do that in other places https://github.com/grafana/grafana/blob/main/public/sass/components/_dashboard_settings.scss#L9, we are mapping them here, but tbh I am not very familiar with the whole themes

$zindex-dropdown: ${theme.zIndex.dropdown};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, nice. i'll do that then 👍

@leeoniya leeoniya requested a review from a team as a code owner October 6, 2023 03:03
@leeoniya leeoniya requested review from sunker and removed request for a team October 6, 2023 03:03
@leeoniya
Copy link
Contributor Author

leeoniya commented Oct 6, 2023

to me this works also in Explore (Chrome, FF - mac), is #75597 (comment) still relevant?
the only "weird" thing is the scrollbar appearing above the tooltip (which doesn't happen in main).

@Elfo404 yes it's still relevant (though i cannot repro it in Linux/Chrome). unfortunately i dont think this can be fixed easily without touching react-custom-scrollbars-2 styling, which is a big can of worms that i don't want to touch here (or at all).

the way we'll aim to solve this in a future PR (once we portal tooltips into panels) is to treat any react-custom-scrollbars-2 container as the bounding box for tooltip positioning. so the tooltip would shift to the left side of the cursor before it overlaps the scrollbar. we will also play around with position: fixed, but that brings back the same scroll-related positioning headaches as the current approach of a global portalling.

btw, did you have to manually switch out the hardcoded tooltip mode to TooltipDisplayMode.Multi? i'm having trouble getting the multi-tooltip to show up in Explore as in your screen rec. my screenshot used the new tooltip model which still ignores the single/multi setting :)

@leeoniya leeoniya merged commit e2dcd51 into main Oct 6, 2023
19 checks passed
@leeoniya leeoniya deleted the leeoniya/splitpane-panel-zindex-overflow branch October 6, 2023 03:46
@Elfo404
Copy link
Member

Elfo404 commented Oct 6, 2023

@leeoniya yep, there's no hidden setting :)

@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
leeoniya added a commit that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/explore area/frontend area/panel/edit no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants