From 6e17955ce2e1f6d2c470202832576763de8f1375 Mon Sep 17 00:00:00 2001
From: Alexander Lesnenko <alxnddr@users.noreply.github.com>
Date: Tue, 15 Feb 2022 17:23:46 +0000
Subject: [PATCH] fix data picker another double scroll and add a visual test
(#20524)
* fix data picker another double scroll and add a visual test
* review
---
.../AccordionList/AccordionList.jsx | 133 +++++++++---------
.../notebook/notebook.cy.spec.js | 25 +++-
2 files changed, 90 insertions(+), 68 deletions(-)
diff --git a/frontend/src/metabase/core/components/AccordionList/AccordionList.jsx b/frontend/src/metabase/core/components/AccordionList/AccordionList.jsx
index 0b7c8485517..84af8fece37 100644
--- a/frontend/src/metabase/core/components/AccordionList/AccordionList.jsx
+++ b/frontend/src/metabase/core/components/AccordionList/AccordionList.jsx
@@ -1,4 +1,5 @@
import React, { Component } from "react";
+import ReactDOM from "react-dom";
import PropTypes from "prop-types";
import { List, CellMeasurer, CellMeasurerCache } from "react-virtualized";
@@ -45,8 +46,6 @@ export default class AccordionList extends Component {
fixedWidth: true,
minHeight: 10,
});
-
- this.containerRef = React.createRef();
}
static propTypes = {
@@ -127,6 +126,8 @@ export default class AccordionList extends Component {
};
componentDidMount() {
+ this.container = ReactDOM.findDOMNode(this);
+
// NOTE: for some reason the row heights aren't computed correctly when
// first rendering, so force the list to update
this._forceUpdateList();
@@ -134,11 +135,11 @@ export default class AccordionList extends Component {
// Use list.scrollToRow instead of the scrollToIndex prop since the
// causes the list's scrolling to be pinned to the selected row
setTimeout(() => {
- const hasFocusedChildren = this.containerRef?.current?.contains(
+ const hasFocusedChildren = this.container.contains(
document.activeElement,
);
if (!hasFocusedChildren && this.props.hasInitialFocus) {
- this.containerRef?.current?.focus();
+ this.container.focus();
}
const index = this._initialSelectedRowIndex;
@@ -272,7 +273,7 @@ export default class AccordionList extends Component {
for (let sectionIndex = 0; sectionIndex < sections.length; sectionIndex++) {
const section = sections[sectionIndex];
- for (let itemIndex = 0; itemIndex < section.items.length; itemIndex++) {
+ for (let itemIndex = 0; itemIndex < section.items?.length; itemIndex++) {
const item = section.items[itemIndex];
if (itemIsSelected(item)) {
return {
@@ -512,6 +513,8 @@ export default class AccordionList extends Component {
);
}
+ isVirtualized = () => this.props.maxHeight !== Infinity;
+
canToggleSections = () => {
const { alwaysTogglable, sections } = this.props;
return alwaysTogglable || sections.length > 1;
@@ -539,7 +542,7 @@ export default class AccordionList extends Component {
// Because of virtualization, focused search input can be removed which does not trigger blur event.
// We need to restore focus on the component root container to make keyboard navigation working
handleSearchRemoval = () => {
- this.containerRef?.current?.focus();
+ this.container?.focus();
};
render() {
@@ -553,11 +556,10 @@ export default class AccordionList extends Component {
const searchRowIndex = rows.findIndex(row => row.type === "search");
- if (this.props.maxHeight === Infinity) {
+ if (!this.isVirtualized()) {
return (
<AccordionListRoot
role="tree"
- ref={this.containerRef}
onKeyDown={this.handleKeyDown}
tabIndex={-1}
className={className}
@@ -603,68 +605,65 @@ export default class AccordionList extends Component {
// HACK - Ensure the component can scroll
// This is a temporary fix to handle cases where the parent component doesn’t pass in the correct `maxHeight`
overflowY: "auto",
+ outline: "none",
};
return (
- <AccordionListRoot
- role="tree"
- ref={this.containerRef}
- onKeyDown={this.handleKeyDown}
- tabIndex={-1}
- >
- <List
- id={id}
- ref={list => (this._list = list)}
- className={className}
- style={{ ...defaultListStyle, ...style }}
- containerStyle={{ pointerEvents: "auto" }}
- width={width}
- height={height}
- rowCount={rows.length}
- deferredMeasurementCache={this._cache}
- rowHeight={this._cache.rowHeight}
- // HACK: needs to be large enough to render enough rows to fill the screen since we used
- // the CellMeasurerCache to calculate the height
- overscanRowCount={100}
- scrollToIndex={scrollToIndex}
- scrollToAlignment={scrollToAlignment}
- rowRenderer={({ key, index, parent, style }) => {
- return (
- <CellMeasurer
- cache={this._cache}
- columnIndex={0}
- key={key}
- rowIndex={index}
- parent={parent}
- >
- {({ measure }) => (
- <AccordionListCell
- hasCursor={this.isRowSelected(rows[index])}
- {...this.props}
- style={style}
- row={rows[index]}
- sections={sections}
- onChange={this.handleChange}
- searchText={this.state.searchText}
- onChangeSearchText={this.handleChangeSearchText}
- sectionIsExpanded={this.isSectionExpanded}
- canToggleSections={this.canToggleSections()}
- toggleSection={this.toggleSection}
- />
- )}
- </CellMeasurer>
- );
- }}
- onRowsRendered={({ startIndex, stopIndex }) => {
- this._startIndex = startIndex;
- this._stopIndex = stopIndex;
-
- if (searchRowIndex < startIndex || searchRowIndex > stopIndex) {
- this.handleSearchRemoval();
- }
- }}
- />
- </AccordionListRoot>
+ <List
+ id={id}
+ ref={list => (this._list = list)}
+ className={className}
+ style={{ ...defaultListStyle, ...style }}
+ containerStyle={{ pointerEvents: "auto" }}
+ width={width}
+ height={height}
+ rowCount={rows.length}
+ deferredMeasurementCache={this._cache}
+ rowHeight={this._cache.rowHeight}
+ // HACK: needs to be large enough to render enough rows to fill the screen since we used
+ // the CellMeasurerCache to calculate the height
+ overscanRowCount={100}
+ scrollToIndex={scrollToIndex}
+ scrollToAlignment={scrollToAlignment}
+ containerProps={{
+ onKeyDown: this.handleKeyDown,
+ }}
+ rowRenderer={({ key, index, parent, style }) => {
+ return (
+ <CellMeasurer
+ cache={this._cache}
+ columnIndex={0}
+ key={key}
+ rowIndex={index}
+ parent={parent}
+ >
+ {({ measure }) => (
+ <AccordionListCell
+ hasCursor={this.isRowSelected(rows[index])}
+ {...this.props}
+ style={style}
+ row={rows[index]}
+ sections={sections}
+ onChange={this.handleChange}
+ searchText={this.state.searchText}
+ onChangeSearchText={this.handleChangeSearchText}
+ sectionIsExpanded={this.isSectionExpanded}
+ canToggleSections={this.canToggleSections()}
+ toggleSection={this.toggleSection}
+ />
+ )}
+ </CellMeasurer>
+ );
+ }}
+ onRowsRendered={({ startIndex, stopIndex }) => {
+ this._startIndex = startIndex;
+ this._stopIndex = stopIndex;
+
+ if (searchRowIndex < startIndex || searchRowIndex > stopIndex) {
+ this.handleSearchRemoval();
+ }
+ }}
+ />
);
}
}
diff --git a/frontend/test/metabase-visual/notebook/notebook.cy.spec.js b/frontend/test/metabase-visual/notebook/notebook.cy.spec.js
index 956d194d764..afccbb431b3 100644
--- a/frontend/test/metabase-visual/notebook/notebook.cy.spec.js
+++ b/frontend/test/metabase-visual/notebook/notebook.cy.spec.js
@@ -1,4 +1,5 @@
-import { restore, popover } from "__support__/e2e/cypress";
+import _ from "underscore";
+import { restore, popover, openNotebookEditor } from "__support__/e2e/cypress";
describe("visual tests > notebook > major UI elements", () => {
const VIEWPORT_WIDTH = 2500;
@@ -103,6 +104,28 @@ describe("visual tests > notebook > Run buttons", () => {
});
});
+describe("visual tests > notebook", () => {
+ const VIEWPORT_WIDTH = 1200;
+ const VIEWPORT_HEIGHT = 600;
+
+ beforeEach(() => {
+ restore();
+ cy.signInAsAdmin();
+ cy.viewport(VIEWPORT_WIDTH, VIEWPORT_HEIGHT);
+
+ _.range(10).forEach(index => {
+ const name = `Sample Database ${index + 1}`;
+ cy.addH2SampleDatabase({ name });
+ });
+ });
+
+ it("data picker", () => {
+ openNotebookEditor();
+ cy.findByText("Sample Database");
+ cy.percySnapshot();
+ });
+});
+
function selectFromDropdown(itemName) {
return popover().findByText(itemName);
}
--
GitLab