Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Commit

Permalink
Fix 3D panel circular import and Interaction stories (#146)
Browse files Browse the repository at this point in the history
The main problem here was a circular import that caused a component to be imported as `undefined`. I found a webpack plugin that can help us detect these—turned on warnings for now, but there are too many to fix currently. Also the plugin doesn't understand that our async imports are a valid way of breaking cycles (aackerman/circular-dependency-plugin#67).
One of the stories had a timing issue — I looked deeper for how to address this, but I think some problems stem from hacky use of `ref` as a way of detecting `onMount` in class components... ideally we would convert everything here to hooks, but that is too big a task for this PR, so just adding a band-aid on this story for now.

I also found that the Layout stories pertain to enabling map height rendering for the Cruise internal map, so are irrelevant here (reverting part of cruise-automation/webviz@6eb6fd9).
  • Loading branch information
jtbandes authored and amacneil committed Mar 8, 2021
1 parent 1bc4971 commit 4bb6b53
Show file tree
Hide file tree
Showing 12 changed files with 71 additions and 144 deletions.
2 changes: 1 addition & 1 deletion app/panels/ThreeDimensionalViz/CameraInfo/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import {
import {
SValue,
SLabel,
} from "@foxglove-studio/app/panels/ThreeDimensionalViz/Interactions/Interactions";
} from "@foxglove-studio/app/panels/ThreeDimensionalViz/Interactions/styling";
import styles from "@foxglove-studio/app/panels/ThreeDimensionalViz/Layout.module.scss";
import {
getNewCameraStateOnFollowChange,
Expand Down
2 changes: 1 addition & 1 deletion app/panels/ThreeDimensionalViz/DrawingTools/Polygons.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import ValidatedInput, { EditFormat } from "@foxglove-studio/app/components/Vali
import {
SValue,
SLabel,
} from "@foxglove-studio/app/panels/ThreeDimensionalViz/Interactions/Interactions";
} from "@foxglove-studio/app/panels/ThreeDimensionalViz/Interactions/styling";
import {
polygonsToPoints,
getFormattedString,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,7 @@ storiesOf("<Interaction> / open-close behavior", module)
<MarkerStory
onMount={(_) =>
setImmediate(async () => {
await delay(100);
(document.querySelectorAll(
'[data-test="ExpandingToolbar-Drawing tools"]',
)[0] as any).click(); // Start drawing
Expand Down
24 changes: 1 addition & 23 deletions app/panels/ThreeDimensionalViz/Interactions/Interactions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@
import CursorDefault from "@mdi/svg/svg/cursor-default.svg";
import * as React from "react";
import { MouseEventObject } from "regl-worldview";
import styled from "styled-components";

import LinkedGlobalVariableList from "./LinkedGlobalVariableList";
import PointCloudDetails from "./PointCloudDetails";
import { SEmptyState, SRow, SValue } from "./styling";
import useLinkedGlobalVariables from "./useLinkedGlobalVariables";
import Checkbox from "@foxglove-studio/app/components/Checkbox";
import ExpandingToolbar, {
Expand All @@ -32,28 +32,6 @@ import styles from "@foxglove-studio/app/panels/ThreeDimensionalViz/Layout.modul
import { decodeAdditionalFields } from "@foxglove-studio/app/panels/ThreeDimensionalViz/commands/PointClouds/selection";
import { getInteractionData } from "@foxglove-studio/app/panels/ThreeDimensionalViz/threeDimensionalVizUtils";
import { SaveConfig, PanelConfig } from "@foxglove-studio/app/types/panels";
import { colors } from "@foxglove-studio/app/util/sharedStyleConstants";

export const SRow = styled.div`
display: flex;
align-items: center;
padding: 0;
margin: 4px 0;
`;
export const SLabel = styled.label<{ width?: number }>`
width: ${(props) => (props.width ? `${props.width}px` : "80px")};
margin: 4px 0;
font-size: 10px;
`;
export const SValue = styled.div`
color: ${colors.HIGHLIGHT};
word-break: break-word;
`;
export const SEmptyState = styled.div`
color: ${colors.TEXT_MUTED};
line-height: 1.4;
margin-bottom: 8px;
`;

export const OBJECT_TAB_TYPE = "Selected object";
export const LINKED_VARIABLES_TAB_TYPE = "Linked variables";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import * as React from "react";
import styled from "styled-components";

import GlobalVariableLink, { SPath, GlobalVariableName } from "./GlobalVariableLink/index";
import { SEmptyState } from "./index";
import { getPath } from "./interactionUtils";
import { SEmptyState } from "./styling";
import { LinkedGlobalVariables } from "./useLinkedGlobalVariables";
import Icon from "@foxglove-studio/app/components/Icon";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import React, { useMemo, useState, useCallback } from "react";
import { MouseEventObject } from "regl-worldview";
import styled from "styled-components";

import { SValue, SLabel } from "./index";
import { SValue, SLabel } from "./styling";
import ChildToggle from "@foxglove-studio/app/components/ChildToggle";
import Icon from "@foxglove-studio/app/components/Icon";
import Menu from "@foxglove-studio/app/components/Menu";
Expand Down
28 changes: 28 additions & 0 deletions app/panels/ThreeDimensionalViz/Interactions/styling.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/

import styled from "styled-components";

import { colors } from "@foxglove-studio/app/util/sharedStyleConstants";

export const SRow = styled.div`
display: flex;
align-items: center;
padding: 0;
margin: 4px 0;
`;
export const SLabel = styled.label<{ width?: number }>`
width: ${(props) => `${props.width ?? 80}px`};
margin: 4px 0;
font-size: 10px;
`;
export const SValue = styled.div`
color: ${colors.HIGHLIGHT};
word-break: break-word;
`;
export const SEmptyState = styled.div`
color: ${colors.TEXT_MUTED};
line-height: 1.4;
margin-bottom: 8px;
`;
103 changes: 0 additions & 103 deletions app/panels/ThreeDimensionalViz/TopicTree/Layout.stories.tsx

This file was deleted.

17 changes: 4 additions & 13 deletions app/panels/ThreeDimensionalViz/TopicTree/Layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,6 @@ export default function Layout({
selectedTopicNames,
shouldExpandAllKeys,
visibleTopicsCountByKey,
toggleNamespaceChecked,
} = topicTreeData;

useEffect(() => setSubscriptions(selectedTopicNames), [selectedTopicNames, setSubscriptions]);
Expand Down Expand Up @@ -629,23 +628,15 @@ export default function Layout({
cameraState: currentCameraState,
saveConfig: currentSaveConfig,
} = callbackInputsRef.current;
const newPerspective = !currentCameraState.perspective;
currentSaveConfig({ cameraState: { ...currentCameraState, perspective: newPerspective } });
currentSaveConfig({
cameraState: { ...currentCameraState, perspective: !currentCameraState.perspective },
});
if (measuringElRef.current && currentCameraState.perspective) {
measuringElRef.current.reset();
}
// Automatically enable/disable map height based on 3D/2D mode
const mapHeightEnabled = (selectedNamespacesByTopic["/metadata"] || []).includes("height");
if (mapHeightEnabled !== newPerspective) {
toggleNamespaceChecked({
topicName: "/metadata",
namespace: "height",
columnIndex: 0,
});
}
},
};
}, [handleEvent, selectObject, selectedNamespacesByTopic, toggleNamespaceChecked]);
}, [handleEvent, selectObject]);

// When the TopicTree is hidden, focus the <World> again so keyboard controls continue to work
const worldRef = useRef<typeof Worldview | null | undefined>(null);
Expand Down
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,15 @@
"@mdx-js/loader": "1.6.22",
"@sentry/electron": "2.4.0",
"@types/case-sensitive-paths-webpack-plugin": "2.1.4",
"@types/circular-dependency-plugin": "^5",
"@types/electron-devtools-installer": "2.2.0",
"@types/nearley": "2.11.1",
"@types/sass": "^1",
"@types/webpack-dev-server": "3.11.1",
"@typescript-eslint/eslint-plugin": "4.16.1",
"@typescript-eslint/parser": "4.16.1",
"browserify-zlib": "0.2.0",
"circular-dependency-plugin": "5.2.2",
"clean-webpack-plugin": "3.0.0",
"colors": "1.4.0",
"crypto-browserify": "3.12.0",
Expand Down
12 changes: 11 additions & 1 deletion webpack.renderer.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
// file, You can obtain one at http://mozilla.org/MPL/2.0/

import rehypePrism from "@mapbox/rehype-prism";
import CircularDependencyPlugin from "circular-dependency-plugin";
import ForkTsCheckerWebpackPlugin from "fork-ts-checker-webpack-plugin";
import HtmlWebpackPlugin from "html-webpack-plugin";
import path from "path";
import retext from "retext";
import retextSmartypants from "retext-smartypants";
import webpack, { Configuration, EnvironmentPlugin } from "webpack";
import webpack, { Configuration, EnvironmentPlugin, WebpackPluginInstance } from "webpack";

import uncheckedIndexAccessFiles from "./UncheckedIndexAccess.json";
import { WebpackArgv } from "./WebpackArgv";
Expand Down Expand Up @@ -166,6 +167,15 @@ export function makeConfig(_: unknown, argv: WebpackArgv): Configuration {
],
},
plugins: [
new CircularDependencyPlugin({
exclude: /node_modules/,
onDetected({ paths, compilation }) {
if (paths.includes("app/GlobalConfig.ts")) {
return;
}
compilation.warnings.push(new Error(paths.join(" -> ")));
},
}) as WebpackPluginInstance,
new webpack.ProvidePlugin({
// since we avoid "import React from 'react'" we shim here when used globally
React: "react",
Expand Down
20 changes: 20 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3462,6 +3462,15 @@ __metadata:
languageName: node
linkType: hard

"@types/circular-dependency-plugin@npm:^5":
version: 5.0.1
resolution: "@types/circular-dependency-plugin@npm:5.0.1"
dependencies:
"@types/webpack": "*"
checksum: cccbdffa9d1f8ca879c08338c777c9aaf51652c4e1ca5a3a422cf1ad90139d15cdac36ee960f6d7416dcaa312ff81898f5255230b88147efe900d2fb39c7e8e6
languageName: node
linkType: hard

"@types/classnames@npm:2.2.11":
version: 2.2.11
resolution: "@types/classnames@npm:2.2.11"
Expand Down Expand Up @@ -6880,6 +6889,15 @@ __metadata:
languageName: node
linkType: hard

"circular-dependency-plugin@npm:5.2.2":
version: 5.2.2
resolution: "circular-dependency-plugin@npm:5.2.2"
peerDependencies:
webpack: ">=4.0.1"
checksum: 1f6063947017b6b4e4c2111cd097a5bd33e46a20beab07717b2a378641f49c9a5627f89c3e1afee01b032d9758865f61e1c0c11cfc3acaa81068ce900c1cdb81
languageName: node
linkType: hard

"cjs-module-lexer@npm:^0.6.0":
version: 0.6.0
resolution: "cjs-module-lexer@npm:0.6.0"
Expand Down Expand Up @@ -10457,13 +10475,15 @@ __metadata:
"@mdx-js/loader": 1.6.22
"@sentry/electron": 2.4.0
"@types/case-sensitive-paths-webpack-plugin": 2.1.4
"@types/circular-dependency-plugin": ^5
"@types/electron-devtools-installer": 2.2.0
"@types/nearley": 2.11.1
"@types/sass": ^1
"@types/webpack-dev-server": 3.11.1
"@typescript-eslint/eslint-plugin": 4.16.1
"@typescript-eslint/parser": 4.16.1
browserify-zlib: 0.2.0
circular-dependency-plugin: 5.2.2
clean-webpack-plugin: 3.0.0
colors: 1.4.0
crypto-browserify: 3.12.0
Expand Down

0 comments on commit 4bb6b53

Please sign in to comment.