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: Reduce scope of contain: strict to TextPanel #75499

Merged
merged 5 commits into from Sep 27, 2023

Conversation

leeoniya
Copy link
Contributor

@leeoniya leeoniya commented Sep 26, 2023

placeholder for attempt number 3 of #75329

here we will add additional changes to PanelChrome that prevent unwanted overflow/regressions shown in #75466

also, will explore keeping some performance benefits of contain: strict, probably by relaxing to contain: layout size, which omits paint that is part of the strict shorthand: https://css-tricks.com/lets-take-a-deep-dive-into-the-css-contain-property/

@leeoniya leeoniya added area/dashboard no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Sep 26, 2023
@leeoniya leeoniya added this to the 10.2.x milestone Sep 26, 2023
@leeoniya leeoniya self-assigned this Sep 26, 2023
@leeoniya
Copy link
Contributor Author

leeoniya commented Sep 26, 2023

looks like switching to contain: size layout was all that was needed to fix the overflow issues. can't say i fully understand why, though 😅

@leeoniya leeoniya marked this pull request as ready for review September 26, 2023 21:48
@leeoniya leeoniya requested a review from a team as a code owner September 26, 2023 21:48
@leeoniya leeoniya requested review from tskarhed and ashharrison90 and removed request for a team, tskarhed and ashharrison90 September 26, 2023 21:49
Copy link
Member

@ryantxu ryantxu left a comment

Choose a reason for hiding this comment

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

works as expected...
2023-09-26_15-50-16 (1)

also do not understand it 🤷🏻

Copy link
Member

@dprokop dprokop left a comment

Choose a reason for hiding this comment

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

Tested a couple of dev dashboards and haven't noticed any regressions.

But, I've managed to see the problem mentioned by @gtk-grafana that's happening in Explore:

Screenshot 2023-09-27 at 09 26 10

Switching back to main - this problem is not there, so seems like this must be caused by the contain: strict change.

UPDATE: Seems like this happens when the Show more lines .... is shown:
image

UPDATE2: Actually it's broken on main too, but in a different way:

image

contain: strict clips it, but one can not scroll to the bottom of the legend anyways. The problem is here https://github.com/grafana/grafana/blob/main/public/app/features/explore/Graph/ExploreGraph.tsx#L215 - the height of the PanelRenderer does not take into account the Show more lines... being present so basically the DOM element for the panel is pushed outside of the panel container. cc @grafana/explore-squad

Can we just make the Show more lines... fixed height since it's not responsive or anything? And then just subtract that value from the panel height when it's shown.

@leeoniya
Copy link
Contributor Author

cc @gelicia ^^

@leeoniya
Copy link
Contributor Author

leeoniya commented Sep 27, 2023

@grafana/explore-squad has been kind enough to fix the Explore snafu for us in #75587 🎉 , so let's see if this PR sticks this time 😅

@leeoniya leeoniya merged commit 6fc4e93 into main Sep 27, 2023
18 checks passed
@leeoniya leeoniya deleted the leeoniya/contain-loose-2 branch September 27, 2023 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dashboard area/frontend 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