Full page image: fix scrolling with the new native scroll #524
Conversation
| @@ -3,4 +3,5 @@ module.exports = { | |||
| preset: 'ts-jest', | |||
| testEnvironment: 'node', | |||
| reporters: ['<rootDir>/tests/reporter.js'], | |||
| testTimeout: 20000, | |||
There was a problem hiding this comment.
The full-page screenshot takes a bit more time than the panel screenshot so tests are failing without it. The default is 5s, this is setting it to 20s. I think this is reasonable. It could even be more, knowing that the image renderer config timeout is 30s
src/browser/browser.ts
Outdated
| @@ -176,21 +176,22 @@ export class Browser { | |||
| } | |||
|
|
|||
| async scrollToLoadAllPanels(page: puppeteer.Page, options: ImageRenderOptions): Promise<DashboardScrollingResult> { | |||
| const scrollDivSelector = '[class="scrollbar-view"]'; | |||
| const scrollDivSelector = '[class$="scrollbar-view"]'; | |||
There was a problem hiding this comment.
I see this is the main change. Why did it change? Why adding $ fixes it?
There was a problem hiding this comment.
It was caused by the PR mentioned in the description. Before, it was the only class set in the div: https://github.com/grafana/grafana/blob/b382cea9c8ee1dea5e6a61c827c64f97a4eb56a2/packages/grafana-ui/src/components/CustomScrollbar/CustomScrollbar.tsx#L119
And now it's using cx, and there is another class: https://github.com/grafana/grafana/pull/82919/files#diff-2dc0a7361d04ff5e54c5337827cbe6a6bf9478b0fa59c2cdcf363b9afd8370e0R36
The $ indicates it needs to end with scrollbar-view while, without it, it indicates it needs to be exactly equal to the string (see doc).
There was a problem hiding this comment.
I'm a bit concern about using classes? Shouldn't we use ids and modify the code to add an id?
If we decide to use classes, shouldn't it be [class*="scrollbar-view"]. It returns the ones that contains 'scrollbar-view'. Therefore, if the classNames order changes (scrollbar-view class first), it won't be affected
https://www.w3schools.com/cssref/sel_attr_contain.php
There was a problem hiding this comment.
I updated this to both use the syntax you suggested and the ID. As discussed on Slack, we can't use the ID only for now as we need to give time to users to upgrade their Grafana version.
The associated Grafana PR to add the ID is here: grafana/grafana#88214
The scrollbar on the dashboard page will soon be replaced by a native scrollbar (see grafana/grafana#82919). This currently breaks the full dashboard screenshot feature.
This PR fixes that (while still working with the old behavior, as this hasn't been enabled on all instances yet) and adds some tests so we can quickly catch issues like that in the future.
Note: I may be a bit ambitious in creating a dashboard with many different panel types. Let's see if tests are always breaking because of that or if it helps us catch some bugs.
Fixes https://github.com/grafana/grafana-enterprise/issues/6514