You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Mattc1221 (via GitHub)" <gi...@apache.org> on 2023/05/21 05:01:23 UTC
[GitHub] [superset] Mattc1221 opened a new pull request, #24154: feat: Add Deck.gl Contour Layer
Mattc1221 opened a new pull request, #24154:
URL: https://github.com/apache/superset/pull/24154
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Adds the Contour Layer from deck.gl [here](https://deck.gl/docs/api-reference/aggregation-layers/contour-layer)
Related SIP: https://github.com/apache/superset/discussions/23553
_**Design Decisions:**_
The addition of the Contour layer follows the structure of existing deck.gl layers to maintain readability and consistency within the codebase.
_Structure_:
The directory `superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Contour/` contains the Front end structure for the contour layer visualization.
- `controlPanel` configures the form to grab relevant information specified by the user to create and tune the contour layer.
- `Contour.tsx` contains the functions that define the Contour Layer and get relevant data.
- `index.ts` serves as the wrapper for the contour component and provides a metadata function to grab information to describe what this visualization is/does.
The contour layer requires a unique input of "contours". Contours are defined as either `Isolines` or `Isobands` (more information [here](https://deck.gl/docs/api-reference/aggregation-layers/contour-layer)).
- `Isolines` require three fields, `threshold`, `color`, `strokeWidth`. `threshold` defines a value to separate aggregated values on the Map. `strokeWidth` defines the width of the line in pixels that separates these values. `color` is just the color of that line.
- `Isobands` are similar to `Isolines`, except the `threshold` takes 2 values to create polygons on the map instead of lines.
To account for this unique structure, I added a new control panel input component:
The added files for this input component are located under `superset-frontend/src/explore/components/controls/ContourControl/`:
- `types.ts` contains all the types needed for this new component.
- `index.tsx` controls the stateful variables that control adding/editing/removing contours.
- `ContourOption.tsx` displays added contours and an appropriate tooltip.
- `ContourPopoverControl.tsx` defines a popup with appropriate inputs for creating/editing an `isoline` and `isoband`.
- `ContourPopoverTrigger.tsx` wraps the `ContourPopoverControl` and controls when it is displayed.
The Back End code for this feature follows the back end code for other deck.gl visualizations as well, providing functions to structure queried data for the Heatmap component.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
New Visualization Recording:
https://github.com/apache/superset/assets/36670322/6b8da315-23e7-4ed9-a0ce-f0905815a405
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
_**Set up**_
1. Start Superset
2. Click on the `Charts` tab at the top of the screen
3. Click the `+ Chart` button at the top right
4. Load a dataset with geo-positional data (longitude/latitude)
5. Search the charts for `Deck.gl Contour` or click on the `Map` chart category
6. Select `deck.gl Contour` and hit `Create New Chart`
7. Update the `Longitude & Latitude` field to the correct columns
8. Select a `Weight` function
9. Select `Create Chart`
10. There should now be a contour layer present on the screen
- Note: the underlying map component may not be visible if the MAP_BOX_API_KEY is not set. To set it follow the instructions [here](https://superset.apache.org/docs/frequently-asked-questions/#why-is-the-map-not-visible-in-the-geospatial-visualization)
Check that the following cause an immediate update
- Check that increasing/decreasing the `Cell Size` control
- Try adding a new isoline by clicking `Click to add new contour`
- Try adding a new isoband by clicking `Click to add new contour`
- Try clicking an existing contour option and ensure that a popover appears with the appropriate fields pre-filled
- Delet an existing contour option
Other Checks:
- Check that hovering a contour option and check that the tooltip shows appropriate information
- Check that in the contour popover, the submit button is disabled when:
- Not all fields are filled
- All fields are filled, but a non-integer is entered in any of the textfields
- Check all descriptions make sense
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Mattc1221 commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "Mattc1221 (via GitHub)" <gi...@apache.org>.
Mattc1221 commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1636989116
Hey @villebro! I'm wondering if you would be able to take another pass through this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297373767
##########
superset-frontend/src/explore/components/controls/ContourControl/index.tsx:
##########
@@ -0,0 +1,143 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState, useEffect } from 'react';
+import styled from '@emotion/styled';
+import DndSelectLabel from 'src/explore/components/controls/DndColumnSelectControl/DndSelectLabel';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import ContourOption from './ContourOption';
+import { contourType, ContourControlProps } from './types';
+
+const DEFAULT_CONTOURS: contourType[] = [
+ {
+ lowerThreshold: 4,
+ color: { r: 255, g: 0, b: 255, a: 100 },
+ strokeWidth: 1,
+ zIndex: 0,
+ },
+ {
+ lowerThreshold: 5,
+ color: { r: 0, g: 255, b: 0, a: 100 },
+ strokeWidth: 2,
+ zIndex: 1,
+ },
+ {
+ lowerThreshold: 6,
+ upperThreshold: 10,
+ color: { r: 0, g: 0, b: 255, a: 100 },
+ zIndex: 2,
+ },
+];
+
+const NewContourFormatPlaceholder = styled('div')`
+ position: relative;
+ width: calc(100% - 4px);
+ bottom: 16px;
+ left: 0;
+`;
+
+const ContourControl = ({ onChange, ...props }: ContourControlProps) => {
+ const [popoverVisible, setpopoverVisible] = useState(false);
+ const [contours, setContours] = useState<contourType[]>(
+ props?.value ? props?.value : DEFAULT_CONTOURS,
+ );
+
+ useEffect(() => {
+ // add z-index to contours
+ const newContours = contours.map((contour, index) => ({
+ ...contour,
+ zIndex: (index + 1) * 10,
+ }));
+ onChange?.(newContours);
+ }, [onChange, contours]);
+
+ const togglePopover = (visible: boolean) => {
+ setpopoverVisible(visible);
+ };
+
+ const handleClickGhostButton = () => {
+ togglePopover(true);
+ };
+
+ const saveContour = (contour: contourType) => {
+ setContours([...contours, contour]);
+ togglePopover(false);
+ };
+
+ const removeContour = (index: number) => {
+ const newContours = [...contours];
+ newContours.splice(index, 1);
+ setContours(newContours);
+ };
+
+ const onShiftContour = (hoverIndex: number, dragIndex: number) => {
+ const newContours = [...contours];
+ [newContours[hoverIndex], newContours[dragIndex]] = [
+ newContours[dragIndex],
+ newContours[hoverIndex],
+ ];
+ setContours(newContours);
+ };
+
+ const editContour = (contour: contourType, index: number) => {
+ const newContours = [...contours];
+ newContours[index] = contour;
+ setContours(newContours);
+ };
+
+ const valuesRenderer = () =>
+ contours.map((contour, index) => (
+ <ContourOption
+ key={index}
+ saveContour={(newContour: contourType) =>
+ editContour(newContour, index)
+ }
+ contour={contour}
+ index={index}
+ onClose={removeContour}
+ onShift={onShiftContour}
+ />
+ ));
+
+ const ghostButtonText = 'Click to add a contour';
+
+ return (
+ <>
+ <DndSelectLabel
Review Comment:
Clever! We should probably decouple the Label from dnd behaviour so that it's usable without "hacks" like this but that's a thing for another time 🙂
##########
superset-frontend/src/explore/components/controls/ContourControl/index.tsx:
##########
@@ -0,0 +1,143 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState, useEffect } from 'react';
+import styled from '@emotion/styled';
+import DndSelectLabel from 'src/explore/components/controls/DndColumnSelectControl/DndSelectLabel';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import ContourOption from './ContourOption';
+import { contourType, ContourControlProps } from './types';
+
+const DEFAULT_CONTOURS: contourType[] = [
+ {
+ lowerThreshold: 4,
+ color: { r: 255, g: 0, b: 255, a: 100 },
+ strokeWidth: 1,
+ zIndex: 0,
+ },
+ {
+ lowerThreshold: 5,
+ color: { r: 0, g: 255, b: 0, a: 100 },
+ strokeWidth: 2,
+ zIndex: 1,
+ },
+ {
+ lowerThreshold: 6,
+ upperThreshold: 10,
+ color: { r: 0, g: 0, b: 255, a: 100 },
+ zIndex: 2,
+ },
+];
+
+const NewContourFormatPlaceholder = styled('div')`
+ position: relative;
+ width: calc(100% - 4px);
+ bottom: 16px;
+ left: 0;
+`;
+
+const ContourControl = ({ onChange, ...props }: ContourControlProps) => {
+ const [popoverVisible, setpopoverVisible] = useState(false);
+ const [contours, setContours] = useState<contourType[]>(
+ props?.value ? props?.value : DEFAULT_CONTOURS,
+ );
+
+ useEffect(() => {
+ // add z-index to contours
+ const newContours = contours.map((contour, index) => ({
+ ...contour,
+ zIndex: (index + 1) * 10,
+ }));
+ onChange?.(newContours);
+ }, [onChange, contours]);
+
+ const togglePopover = (visible: boolean) => {
+ setpopoverVisible(visible);
+ };
+
+ const handleClickGhostButton = () => {
+ togglePopover(true);
+ };
+
+ const saveContour = (contour: contourType) => {
+ setContours([...contours, contour]);
+ togglePopover(false);
+ };
+
+ const removeContour = (index: number) => {
+ const newContours = [...contours];
+ newContours.splice(index, 1);
+ setContours(newContours);
+ };
+
+ const onShiftContour = (hoverIndex: number, dragIndex: number) => {
+ const newContours = [...contours];
+ [newContours[hoverIndex], newContours[dragIndex]] = [
+ newContours[dragIndex],
+ newContours[hoverIndex],
+ ];
+ setContours(newContours);
+ };
+
+ const editContour = (contour: contourType, index: number) => {
+ const newContours = [...contours];
+ newContours[index] = contour;
+ setContours(newContours);
+ };
+
+ const valuesRenderer = () =>
+ contours.map((contour, index) => (
+ <ContourOption
+ key={index}
+ saveContour={(newContour: contourType) =>
+ editContour(newContour, index)
+ }
+ contour={contour}
+ index={index}
+ onClose={removeContour}
+ onShift={onShiftContour}
+ />
+ ));
+
+ const ghostButtonText = 'Click to add a contour';
+
+ return (
+ <>
+ <DndSelectLabel
Review Comment:
Clever! We should probably decouple the Label from dnd behaviour so that it's usable without "hacks" like this but that's a thing for another time 🙂
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Mattc1221 commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "Mattc1221 (via GitHub)" <gi...@apache.org>.
Mattc1221 commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1596245789
@villebro @rusackas @kgabryje @justinpark @michael-s-molina
I was wondering what an ETA on the timeline for review would be? Thanks again!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1234760262
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourPopoverControl.tsx:
##########
@@ -0,0 +1,311 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect } from 'react';
+import { Row, Col } from 'src/components';
+import Button from 'src/components/Button';
+import Tabs from 'src/components/Tabs';
+import { legacyValidateInteger, styled, t } from '@superset-ui/core';
+import ControlHeader from '../../ControlHeader';
+import TextControl from '../TextControl';
+import ColorPickerControl from '../ColorPickerControl';
+import {
+ ContourPopoverControlProps,
+ colorType,
+ contourType,
+ errorMapType,
+} from './types';
+
+enum CONTOUR_TYPES {
+ ISOLINE = 'ISOLINE',
+ ISOBAND = 'ISOBAND',
+}
Review Comment:
nit: PascalCase tends to be used for enums, at least in the official TS docs: https://www.typescriptlang.org/docs/handbook/enums.html#string-enums
```suggestion
enum CONTOUR_TYPES {
Isoline = 'ISOLINE',
Isoband = 'ISOBAND',
}
```
##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -108,6 +108,8 @@ class CeleryConfig(object):
SQLLAB_CTAS_NO_LIMIT = True
+MAPBOX_API_KEY = os.getenv("MAPBOX_API_KEY")
+
Review Comment:
We appear to be using `get_env_variable` in this file, maybe use that here, too, for consistency?
```suggestion
MAPBOX_API_KEY = get_env_variable("MAPBOX_API_KEY")
```
##########
superset-frontend/src/explore/components/controls/TextControl/index.tsx:
##########
@@ -30,7 +30,7 @@ export interface TextControlProps<T extends InputValueType = InputValueType> {
disabled?: boolean;
isFloat?: boolean;
isInt?: boolean;
- onChange?: (value: T, errors: any) => {};
+ onChange?: (value: T, errors: any) => void;
Review Comment:
good catch, thanks for improving this 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297354844
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourPopoverControl.tsx:
##########
@@ -0,0 +1,311 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect } from 'react';
+import { Row, Col } from 'src/components';
+import Button from 'src/components/Button';
+import Tabs from 'src/components/Tabs';
+import { legacyValidateInteger, styled, t } from '@superset-ui/core';
+import ControlHeader from '../../ControlHeader';
+import TextControl from '../TextControl';
+import ColorPickerControl from '../ColorPickerControl';
+import {
+ ContourPopoverControlProps,
+ colorType,
+ contourType,
+ errorMapType,
+} from './types';
+
+enum CONTOUR_TYPES {
+ Isoline = 'ISOLINE',
+ Isoband = 'ISOBAND',
+}
+
+const ContourActionsContainer = styled.div`
+ margin-top: ${({ theme }) => theme.gridUnit * 2}px;
+`;
+
+const StyledRow = styled(Row)`
+ width: 100%;
+ gap: 10px;
+`;
+
+const isIsoband = (contour: contourType) => {
+ if (Object.keys(contour).length < 4) {
+ return false;
+ }
+ return contour.upperThreshold && contour.lowerThreshold;
+};
+
+const getTabKey = (contour: contourType | undefined) =>
+ contour && isIsoband(contour) ? CONTOUR_TYPES.Isoband : CONTOUR_TYPES.Isoline;
+
+const determineErrorMap = (contour: contourType) => {
+ const type = getTabKey(contour);
+ const errorMap: errorMapType = {
+ lowerThreshold: [],
+ upperThreshold: [],
+ strokeWidth: [],
+ color: [],
+ };
+ if (type === CONTOUR_TYPES.Isoband) {
+ const upperThresholdError = legacyValidateInteger(contour.upperThreshold);
+ if (upperThresholdError) errorMap.upperThreshold.push(upperThresholdError);
+ }
+ const lowerThresholdError = legacyValidateInteger(contour.lowerThreshold);
+ const strokeWidthError = legacyValidateInteger(contour.strokeWidth);
+ if (lowerThresholdError) errorMap.lowerThreshold.push(lowerThresholdError);
+ if (strokeWidthError) errorMap.strokeWidth.push(strokeWidthError);
+ return errorMap;
+};
+
+const convertContourToNumeric = (contour: contourType) => {
+ const formattedContour = { ...contour };
+ const numericKeys = ['lowerThreshold', 'upperThreshold', 'strokeWidth'];
+ numericKeys.forEach(key => {
+ formattedContour[key] = Number(formattedContour[key]);
+ });
+ return formattedContour;
+};
+
+const DEFAULT_CONTOUR = {
+ lowerThreshold: undefined,
+ upperThreshold: undefined,
+ color: undefined,
+ strokeWidth: undefined,
+};
+
+const ContourPopoverControl = ({
+ value: initialValue,
+ onSave,
+ onClose,
+}: ContourPopoverControlProps) => {
+ const [currentTab, setCurrentTab] = useState(getTabKey(initialValue));
+ const [contour, setContour] = useState(initialValue || DEFAULT_CONTOUR);
+ const [validationErrors, setValidationErrors] = useState(
+ determineErrorMap(initialValue || DEFAULT_CONTOUR),
+ );
+ const [isComplete, setIsComplete] = useState(false);
+
+ useEffect(() => {
+ const isIsoband = currentTab === CONTOUR_TYPES.Isoband;
+ const validLower =
+ Boolean(contour.lowerThreshold) || contour.lowerThreshold === 0;
+ const validUpper =
+ Boolean(contour.upperThreshold) || contour.upperThreshold === 0;
+ const validStrokeWidth =
+ Boolean(contour.strokeWidth) || contour.strokeWidth === 0;
+ const validColor =
+ typeof contour.color === 'object' &&
+ 'r' in contour.color &&
+ typeof contour.color.r === 'number' &&
+ 'g' in contour.color &&
+ typeof contour.color.g === 'number' &&
+ 'b' in contour.color &&
+ typeof contour.color.b === 'number' &&
+ 'a' in contour.color &&
+ typeof contour.color.a === 'number';
+
+ const errors = determineErrorMap(contour);
+ setValidationErrors(errors);
+
+ setIsComplete(
+ isIsoband
+ ? validLower && validUpper && validColor
+ : validLower && validColor && validStrokeWidth,
+ );
+ }, [contour, currentTab]);
+
+ const onTabChange = (activeKey: any) => {
+ setCurrentTab(activeKey);
+ };
+
+ const updateStrokeWidth = (value: number | string) => {
+ const newContour = { ...contour };
+ newContour.strokeWidth = value;
+ setContour(newContour);
+ };
+
+ const updateColor = (rgb: colorType) => {
+ const newContour = { ...contour };
+ newContour.color = { ...rgb, a: 100 };
+ setContour(newContour);
+ };
+
+ const updateLowerThreshold = (value: number | string) => {
+ const newContour = { ...contour };
+ newContour.lowerThreshold = value;
+ setContour(newContour);
+ };
+
+ const updateUpperThreshold = (value: number | string) => {
+ const newContour = { ...contour };
+ newContour.upperThreshold = value;
+ setContour(newContour);
+ };
+
+ const containsErrors = () => {
+ const keys = Object.keys(validationErrors);
+ return keys.some(key => validationErrors[key].length > 0);
+ };
+
+ const handleSave = () => {
+ if (isComplete && onSave) {
+ onSave(convertContourToNumeric(contour));
+ }
+ };
+
+ const isobandSection = () => (
+ <div className="isoline-popover-section">
+ <StyledRow>
+ <Col flex="1">
+ <ControlHeader
+ name="isoband-threshold-lower"
+ label="Lower Threshold"
Review Comment:
Please add translations for labels and descriptions in isobandSection and isolineSection 🙏
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourPopoverControl.tsx:
##########
@@ -0,0 +1,311 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect } from 'react';
+import { Row, Col } from 'src/components';
+import Button from 'src/components/Button';
+import Tabs from 'src/components/Tabs';
+import { legacyValidateInteger, styled, t } from '@superset-ui/core';
+import ControlHeader from '../../ControlHeader';
+import TextControl from '../TextControl';
+import ColorPickerControl from '../ColorPickerControl';
+import {
+ ContourPopoverControlProps,
+ colorType,
+ contourType,
+ errorMapType,
+} from './types';
+
+enum CONTOUR_TYPES {
+ Isoline = 'ISOLINE',
+ Isoband = 'ISOBAND',
+}
+
+const ContourActionsContainer = styled.div`
+ margin-top: ${({ theme }) => theme.gridUnit * 2}px;
+`;
+
+const StyledRow = styled(Row)`
+ width: 100%;
+ gap: 10px;
+`;
+
+const isIsoband = (contour: contourType) => {
+ if (Object.keys(contour).length < 4) {
+ return false;
+ }
+ return contour.upperThreshold && contour.lowerThreshold;
+};
+
+const getTabKey = (contour: contourType | undefined) =>
+ contour && isIsoband(contour) ? CONTOUR_TYPES.Isoband : CONTOUR_TYPES.Isoline;
+
+const determineErrorMap = (contour: contourType) => {
+ const type = getTabKey(contour);
+ const errorMap: errorMapType = {
+ lowerThreshold: [],
+ upperThreshold: [],
+ strokeWidth: [],
+ color: [],
+ };
+ if (type === CONTOUR_TYPES.Isoband) {
+ const upperThresholdError = legacyValidateInteger(contour.upperThreshold);
+ if (upperThresholdError) errorMap.upperThreshold.push(upperThresholdError);
+ }
+ const lowerThresholdError = legacyValidateInteger(contour.lowerThreshold);
+ const strokeWidthError = legacyValidateInteger(contour.strokeWidth);
+ if (lowerThresholdError) errorMap.lowerThreshold.push(lowerThresholdError);
+ if (strokeWidthError) errorMap.strokeWidth.push(strokeWidthError);
+ return errorMap;
+};
+
+const convertContourToNumeric = (contour: contourType) => {
+ const formattedContour = { ...contour };
+ const numericKeys = ['lowerThreshold', 'upperThreshold', 'strokeWidth'];
+ numericKeys.forEach(key => {
+ formattedContour[key] = Number(formattedContour[key]);
+ });
+ return formattedContour;
+};
+
+const DEFAULT_CONTOUR = {
+ lowerThreshold: undefined,
+ upperThreshold: undefined,
+ color: undefined,
+ strokeWidth: undefined,
+};
+
+const ContourPopoverControl = ({
+ value: initialValue,
+ onSave,
+ onClose,
+}: ContourPopoverControlProps) => {
+ const [currentTab, setCurrentTab] = useState(getTabKey(initialValue));
+ const [contour, setContour] = useState(initialValue || DEFAULT_CONTOUR);
+ const [validationErrors, setValidationErrors] = useState(
+ determineErrorMap(initialValue || DEFAULT_CONTOUR),
+ );
+ const [isComplete, setIsComplete] = useState(false);
+
+ useEffect(() => {
+ const isIsoband = currentTab === CONTOUR_TYPES.Isoband;
+ const validLower =
+ Boolean(contour.lowerThreshold) || contour.lowerThreshold === 0;
+ const validUpper =
+ Boolean(contour.upperThreshold) || contour.upperThreshold === 0;
+ const validStrokeWidth =
+ Boolean(contour.strokeWidth) || contour.strokeWidth === 0;
+ const validColor =
+ typeof contour.color === 'object' &&
+ 'r' in contour.color &&
+ typeof contour.color.r === 'number' &&
+ 'g' in contour.color &&
+ typeof contour.color.g === 'number' &&
+ 'b' in contour.color &&
+ typeof contour.color.b === 'number' &&
+ 'a' in contour.color &&
+ typeof contour.color.a === 'number';
+
+ const errors = determineErrorMap(contour);
+ setValidationErrors(errors);
+
+ setIsComplete(
+ isIsoband
+ ? validLower && validUpper && validColor
+ : validLower && validColor && validStrokeWidth,
+ );
+ }, [contour, currentTab]);
+
+ const onTabChange = (activeKey: any) => {
+ setCurrentTab(activeKey);
+ };
+
+ const updateStrokeWidth = (value: number | string) => {
+ const newContour = { ...contour };
+ newContour.strokeWidth = value;
+ setContour(newContour);
+ };
+
+ const updateColor = (rgb: colorType) => {
+ const newContour = { ...contour };
+ newContour.color = { ...rgb, a: 100 };
+ setContour(newContour);
+ };
+
+ const updateLowerThreshold = (value: number | string) => {
+ const newContour = { ...contour };
+ newContour.lowerThreshold = value;
+ setContour(newContour);
+ };
+
+ const updateUpperThreshold = (value: number | string) => {
+ const newContour = { ...contour };
+ newContour.upperThreshold = value;
+ setContour(newContour);
+ };
+
+ const containsErrors = () => {
+ const keys = Object.keys(validationErrors);
+ return keys.some(key => validationErrors[key].length > 0);
+ };
+
+ const handleSave = () => {
+ if (isComplete && onSave) {
+ onSave(convertContourToNumeric(contour));
+ }
+ };
+
+ const isobandSection = () => (
+ <div className="isoline-popover-section">
+ <StyledRow>
+ <Col flex="1">
+ <ControlHeader
+ name="isoband-threshold-lower"
+ label="Lower Threshold"
Review Comment:
Please add translations for labels and descriptions in isobandSection and isolineSection 🙏
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297358403
##########
superset-frontend/src/explore/components/controls/ContourControl/index.tsx:
##########
@@ -0,0 +1,143 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState, useEffect } from 'react';
+import styled from '@emotion/styled';
+import DndSelectLabel from 'src/explore/components/controls/DndColumnSelectControl/DndSelectLabel';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import ContourOption from './ContourOption';
+import { contourType, ContourControlProps } from './types';
+
+const DEFAULT_CONTOURS: contourType[] = [
+ {
+ lowerThreshold: 4,
+ color: { r: 255, g: 0, b: 255, a: 100 },
+ strokeWidth: 1,
+ zIndex: 0,
+ },
+ {
+ lowerThreshold: 5,
+ color: { r: 0, g: 255, b: 0, a: 100 },
+ strokeWidth: 2,
+ zIndex: 1,
+ },
+ {
+ lowerThreshold: 6,
+ upperThreshold: 10,
+ color: { r: 0, g: 0, b: 255, a: 100 },
+ zIndex: 2,
+ },
+];
+
+const NewContourFormatPlaceholder = styled('div')`
+ position: relative;
+ width: calc(100% - 4px);
+ bottom: 16px;
+ left: 0;
+`;
+
+const ContourControl = ({ onChange, ...props }: ContourControlProps) => {
+ const [popoverVisible, setpopoverVisible] = useState(false);
Review Comment:
```suggestion
const [popoverVisible, setPopoverVisible] = useState(false);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Mattc1221 commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "Mattc1221 (via GitHub)" <gi...@apache.org>.
Mattc1221 commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1305121705
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourOption.tsx:
##########
@@ -0,0 +1,101 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled } from '@superset-ui/core';
+import { ContourOptionProps } from './types';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import OptionWrapper from '../DndColumnSelectControl/OptionWrapper';
+
+const StyledOptionWrapper = styled(OptionWrapper)`
+ max-width: 100%;
+ min-width: 100%;
+`;
+
+const StyledListItem = styled.li`
+ display: flex;
+ align-items: center;
+`;
+
+const ColorPatch = styled.div<{ formattedColor: string }>`
+ background-color: ${({ formattedColor }) => formattedColor};
+ height: 5px;
Review Comment:
Thanks for the pointer! I wasn't aware of theme grid units!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297374088
##########
superset-frontend/src/explore/components/controls/ContourControl/types.ts:
##########
@@ -0,0 +1,55 @@
+import { OptionValueType } from 'src/explore/components/controls/DndColumnSelectControl/types';
+import { ControlComponentProps } from 'src/explore/components/Control';
+
+export interface colorType {
Review Comment:
Could you use PascalCase for types?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] JohnDietrich-Pepper commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "JohnDietrich-Pepper (via GitHub)" <gi...@apache.org>.
JohnDietrich-Pepper commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1680843721
@villebro @kgabryje @rusackas @michael-s-molina any chance someone could take a look at this. Would be nice to have this functionality and it appears that it has been ready for re-review for about a month and a half now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1556113235
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24154](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (bf480ae) into [master](https://app.codecov.io/gh/apache/superset/commit/1670275571ddd677634bf0ba9b171c6a7275091a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (1670275) will **decrease** coverage by `11.21%`.
> The diff coverage is `64.13%`.
> :exclamation: Current head bf480ae differs from pull request most recent head db2414f. Consider uploading reports for the commit db2414f to get more accurate results
```diff
@@ Coverage Diff @@
## master #24154 +/- ##
===========================================
- Coverage 68.23% 57.02% -11.21%
===========================================
Files 1942 1952 +10
Lines 75205 75376 +171
Branches 8145 8208 +63
===========================================
- Hits 51314 42984 -8330
- Misses 21806 30286 +8480
- Partials 2085 2106 +21
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.23% <32.22%> (+0.13%)` | :arrow_up: |
| python | `59.52% <44.54%> (-23.27%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `53.28% <35.54%> (+0.22%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...set-ui-core/src/ui-overrides/ExtensionsRegistry.ts](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdWktb3ZlcnJpZGVzL0V4dGVuc2lvbnNSZWdpc3RyeS50cw==) | `100.00% <ø> (ø)` | |
| [...ackages/superset-ui-core/src/utils/featureFlags.ts](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdXRpbHMvZmVhdHVyZUZsYWdzLnRz) | `100.00% <ø> (ø)` | |
| [...plugins/legacy-preset-chart-deckgl/src/factory.tsx](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcHJlc2V0LWNoYXJ0LWRlY2tnbC9zcmMvZmFjdG9yeS50c3g=) | `0.00% <ø> (ø)` | |
| [...d/plugins/legacy-preset-chart-deckgl/src/preset.js](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcHJlc2V0LWNoYXJ0LWRlY2tnbC9zcmMvcHJlc2V0Lmpz) | `100.00% <ø> (ø)` | |
| [...hart-echarts/src/MixedTimeseries/transformProps.ts](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvTWl4ZWRUaW1lc2VyaWVzL3RyYW5zZm9ybVByb3BzLnRz) | `0.00% <0.00%> (ø)` | |
| [...lugin-chart-echarts/src/Timeseries/transformers.ts](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy90cmFuc2Zvcm1lcnMudHM=) | `56.95% <ø> (-0.29%)` | :arrow_down: |
| [...n-chart-handlebars/src/plugin/controls/columns.tsx](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtaGFuZGxlYmFycy9zcmMvcGx1Z2luL2NvbnRyb2xzL2NvbHVtbnMudHN4) | `15.78% <0.00%> (+0.78%)` | :arrow_up: |
| [superset-frontend/src/components/Modal/Modal.tsx](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTW9kYWwvTW9kYWwudHN4) | `85.89% <ø> (ø)` | |
| [...et-frontend/src/components/TableSelector/index.tsx](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVGFibGVTZWxlY3Rvci9pbmRleC50c3g=) | `81.08% <ø> (ø)` | |
| [superset-frontend/src/dashboard/actions/hydrate.js](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL2h5ZHJhdGUuanM=) | `2.04% <0.00%> (-0.05%)` | :arrow_down: |
| ... and [125 more](https://app.codecov.io/gh/apache/superset/pull/24154?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [236 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24154/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1682485019
I think changing the contour type between isoline and isoband doesn't work well.
1. I can change the contour type from isoline to isoband, but I can't do the opposite - when I edit an isoband, change the tab to isoline and type value, it only edits the lower threshold of isoband. I think user could expect that the contour becomes an isoline.
2. The popover should close after clicking save
3. Maybe we should have some kind of validation to ensure that upper threshold is larger than lower threshold?
https://github.com/apache/superset/assets/15073128/d00d4266-c512-435f-9b61-fea0aba5428f
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297346053
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourOption.tsx:
##########
@@ -0,0 +1,101 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled } from '@superset-ui/core';
+import { ContourOptionProps } from './types';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import OptionWrapper from '../DndColumnSelectControl/OptionWrapper';
+
+const StyledOptionWrapper = styled(OptionWrapper)`
+ max-width: 100%;
+ min-width: 100%;
+`;
+
+const StyledListItem = styled.li`
+ display: flex;
+ align-items: center;
+`;
+
+const ColorPatch = styled.div<{ formattedColor: string }>`
+ background-color: ${({ formattedColor }) => formattedColor};
+ height: 5px;
+ width: 5px;
+ margin: 0 5px;
+`;
+
+const ContourOption = ({
+ contour,
+ index,
+ saveContour,
+ onClose,
+ onShift,
+}: ContourOptionProps) => {
+ const { lowerThreshold, upperThreshold, color, strokeWidth } = contour;
+
+ const isIsoband = upperThreshold;
+
+ const formattedColor = color
+ ? `rgba(${color.r}, ${color.g}, ${color.b}, 1)`
+ : 'undefined';
+
+ const formatIsoline = (threshold: number, width: any) =>
+ `Threshold: ${threshold}, color: ${formattedColor}, stroke width: ${width}`;
+
+ const formatIsoband = (threshold: number[]) =>
+ `Threshold: [${threshold[0]}, ${threshold[1]}], color: ${formattedColor}`;
+
+ const displayString = isIsoband
+ ? formatIsoband([lowerThreshold || -1, upperThreshold])
+ : formatIsoline(lowerThreshold || -1, strokeWidth);
+
+ const overlay = (
+ <div className="contour-tooltip-overlay">
+ <StyledListItem>
+ Threshold:{' '}
Review Comment:
Translation here and lines 72, 77 and 80
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297345186
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourOption.tsx:
##########
@@ -0,0 +1,101 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled } from '@superset-ui/core';
+import { ContourOptionProps } from './types';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import OptionWrapper from '../DndColumnSelectControl/OptionWrapper';
+
+const StyledOptionWrapper = styled(OptionWrapper)`
+ max-width: 100%;
+ min-width: 100%;
+`;
+
+const StyledListItem = styled.li`
+ display: flex;
+ align-items: center;
+`;
+
+const ColorPatch = styled.div<{ formattedColor: string }>`
+ background-color: ${({ formattedColor }) => formattedColor};
+ height: 5px;
+ width: 5px;
+ margin: 0 5px;
+`;
+
+const ContourOption = ({
+ contour,
+ index,
+ saveContour,
+ onClose,
+ onShift,
+}: ContourOptionProps) => {
+ const { lowerThreshold, upperThreshold, color, strokeWidth } = contour;
+
+ const isIsoband = upperThreshold;
+
+ const formattedColor = color
+ ? `rgba(${color.r}, ${color.g}, ${color.b}, 1)`
+ : 'undefined';
+
+ const formatIsoline = (threshold: number, width: any) =>
Review Comment:
Could you add type for width? I suppose it's either a number or string?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297450346
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Contour/Contour.tsx:
##########
@@ -0,0 +1,97 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ContourLayer } from 'deck.gl';
+import React from 'react';
+import { t } from '@superset-ui/core';
+import { commonLayerProps } from '../common';
+import sandboxedEval from '../../utils/sandbox';
+import { createDeckGLComponent, getLayerType } from '../../factory';
+import TooltipRow from '../../TooltipRow';
+
+function setTooltipContent(o: any) {
+ return (
+ <div className="deckgl-tooltip">
+ <TooltipRow
+ label={t('Centroid (Longitude and Latitude): ')}
+ value={`(${o.coordinate[0]}, ${o.coordinate[1]})`}
+ />
+ </div>
+ );
+}
+export const getLayer: getLayerType<unknown> = (
+ formData,
+ payload,
+ setTooltip,
+) => {
+ const fd = formData;
+ const {
+ aggregation = 'SUM',
+ js_data_mutator: jsFnMutator,
+ contours: rawContours,
+ cellSize = '200',
+ } = fd;
+ let data = payload.data.features;
+
+ const contours = rawContours?.map(
+ (contour: {
+ color: any;
+ lowerThreshold: number;
+ upperThreshold?: number;
+ strokeWidth?: number;
+ }) => {
+ const { lowerThreshold, upperThreshold, color, strokeWidth } = contour;
+ if (upperThreshold) {
+ // Isoband format
+ return {
+ threshold: [lowerThreshold, upperThreshold],
+ color: [color.r, color.g, color.b],
+ };
+ }
+ // Isoline format
+ return {
+ threshold: lowerThreshold,
+ color: [color.r, color.g, color.b],
+ strokeWidth,
+ };
+ },
+ );
+
+ if (jsFnMutator) {
+ // Applying user defined data mutator if defined
+ const jsFnMutatorFunction = sandboxedEval(fd.js_data_mutator);
+ data = jsFnMutatorFunction(data);
+ }
+
+ return new ContourLayer({
+ id: `contourLayer-${fd.slice_id}`,
+ data,
+ contours,
+ cellSize: Number(cellSize || '200'),
+ aggregation: aggregation.toUpperCase(),
+ getPosition: (d: { position: number[]; weight: number }) => d.position,
+ getWeight: (d: { weight: any }) => d.weight || 0,
Review Comment:
```suggestion
getWeight: (d: { weight: number }) => d.weight || 0,
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297376907
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Contour/Contour.tsx:
##########
@@ -0,0 +1,97 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ContourLayer } from 'deck.gl';
+import React from 'react';
+import { t } from '@superset-ui/core';
+import { commonLayerProps } from '../common';
+import sandboxedEval from '../../utils/sandbox';
+import { createDeckGLComponent, getLayerType } from '../../factory';
+import TooltipRow from '../../TooltipRow';
+
+function setTooltipContent(o: any) {
+ return (
+ <div className="deckgl-tooltip">
+ <TooltipRow
+ label={t('Centroid (Longitude and Latitude): ')}
+ value={`(${o.coordinate[0]}, ${o.coordinate[1]})`}
+ />
+ </div>
+ );
+}
+export const getLayer: getLayerType<unknown> = (
+ formData,
+ payload,
+ setTooltip,
+) => {
+ const fd = formData;
+ const {
+ aggregation = 'SUM',
+ js_data_mutator: jsFnMutator,
+ contours: rawContours,
+ cellSize = '200',
+ } = fd;
+ let data = payload.data.features;
+
+ const contours = rawContours?.map(
+ (contour: {
+ color: any;
Review Comment:
Can we use ColorType here instead of any?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Mattc1221 commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "Mattc1221 (via GitHub)" <gi...@apache.org>.
Mattc1221 commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1305112115
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourOption.tsx:
##########
@@ -0,0 +1,101 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled } from '@superset-ui/core';
+import { ContourOptionProps } from './types';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import OptionWrapper from '../DndColumnSelectControl/OptionWrapper';
+
+const StyledOptionWrapper = styled(OptionWrapper)`
+ max-width: 100%;
+ min-width: 100%;
+`;
+
+const StyledListItem = styled.li`
+ display: flex;
+ align-items: center;
+`;
+
+const ColorPatch = styled.div<{ formattedColor: string }>`
+ background-color: ${({ formattedColor }) => formattedColor};
+ height: 5px;
+ width: 5px;
+ margin: 0 5px;
+`;
+
+const ContourOption = ({
+ contour,
+ index,
+ saveContour,
+ onClose,
+ onShift,
+}: ContourOptionProps) => {
+ const { lowerThreshold, upperThreshold, color, strokeWidth } = contour;
+
+ const isIsoband = upperThreshold;
+
+ const formattedColor = color
+ ? `rgba(${color.r}, ${color.g}, ${color.b}, 1)`
+ : 'undefined';
+
+ const formatIsoline = (threshold: number, width: any) =>
+ `Threshold: ${threshold}, color: ${formattedColor}, stroke width: ${width}`;
+
+ const formatIsoband = (threshold: number[]) =>
+ `Threshold: [${threshold[0]}, ${threshold[1]}], color: ${formattedColor}`;
+
+ const displayString = isIsoband
+ ? formatIsoband([lowerThreshold || -1, upperThreshold])
+ : formatIsoline(lowerThreshold || -1, strokeWidth);
+
+ const overlay = (
+ <div className="contour-tooltip-overlay">
+ <StyledListItem>
+ Threshold:{' '}
+ {isIsoband
+ ? `[${lowerThreshold}, ${upperThreshold}]`
+ : `${lowerThreshold}`}
+ </StyledListItem>
+ <StyledListItem>
+ Color: <ColorPatch formattedColor={formattedColor} /> {formattedColor}
+ </StyledListItem>
+ {!isIsoband && (
+ <StyledListItem>Stroke Width: {strokeWidth}</StyledListItem>
+ )}
+ </div>
+ );
+
+ return (
+ <ContourPopoverTrigger saveContour={saveContour} value={contour}>
+ <StyledOptionWrapper
+ key={index}
Review Comment:
Good catch! this is not required
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297355443
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourPopoverControl.tsx:
##########
@@ -0,0 +1,311 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect } from 'react';
+import { Row, Col } from 'src/components';
+import Button from 'src/components/Button';
+import Tabs from 'src/components/Tabs';
+import { legacyValidateInteger, styled, t } from '@superset-ui/core';
+import ControlHeader from '../../ControlHeader';
+import TextControl from '../TextControl';
+import ColorPickerControl from '../ColorPickerControl';
+import {
+ ContourPopoverControlProps,
+ colorType,
+ contourType,
+ errorMapType,
+} from './types';
+
+enum CONTOUR_TYPES {
+ Isoline = 'ISOLINE',
+ Isoband = 'ISOBAND',
+}
+
+const ContourActionsContainer = styled.div`
+ margin-top: ${({ theme }) => theme.gridUnit * 2}px;
+`;
+
+const StyledRow = styled(Row)`
+ width: 100%;
+ gap: 10px;
+`;
+
+const isIsoband = (contour: contourType) => {
+ if (Object.keys(contour).length < 4) {
+ return false;
+ }
+ return contour.upperThreshold && contour.lowerThreshold;
+};
+
+const getTabKey = (contour: contourType | undefined) =>
+ contour && isIsoband(contour) ? CONTOUR_TYPES.Isoband : CONTOUR_TYPES.Isoline;
+
+const determineErrorMap = (contour: contourType) => {
+ const type = getTabKey(contour);
+ const errorMap: errorMapType = {
+ lowerThreshold: [],
+ upperThreshold: [],
+ strokeWidth: [],
+ color: [],
+ };
+ if (type === CONTOUR_TYPES.Isoband) {
+ const upperThresholdError = legacyValidateInteger(contour.upperThreshold);
+ if (upperThresholdError) errorMap.upperThreshold.push(upperThresholdError);
+ }
+ const lowerThresholdError = legacyValidateInteger(contour.lowerThreshold);
+ const strokeWidthError = legacyValidateInteger(contour.strokeWidth);
+ if (lowerThresholdError) errorMap.lowerThreshold.push(lowerThresholdError);
+ if (strokeWidthError) errorMap.strokeWidth.push(strokeWidthError);
+ return errorMap;
+};
+
+const convertContourToNumeric = (contour: contourType) => {
+ const formattedContour = { ...contour };
+ const numericKeys = ['lowerThreshold', 'upperThreshold', 'strokeWidth'];
+ numericKeys.forEach(key => {
+ formattedContour[key] = Number(formattedContour[key]);
+ });
+ return formattedContour;
+};
+
+const DEFAULT_CONTOUR = {
+ lowerThreshold: undefined,
+ upperThreshold: undefined,
+ color: undefined,
+ strokeWidth: undefined,
+};
+
+const ContourPopoverControl = ({
+ value: initialValue,
+ onSave,
+ onClose,
+}: ContourPopoverControlProps) => {
+ const [currentTab, setCurrentTab] = useState(getTabKey(initialValue));
+ const [contour, setContour] = useState(initialValue || DEFAULT_CONTOUR);
+ const [validationErrors, setValidationErrors] = useState(
+ determineErrorMap(initialValue || DEFAULT_CONTOUR),
+ );
+ const [isComplete, setIsComplete] = useState(false);
+
+ useEffect(() => {
+ const isIsoband = currentTab === CONTOUR_TYPES.Isoband;
+ const validLower =
+ Boolean(contour.lowerThreshold) || contour.lowerThreshold === 0;
+ const validUpper =
+ Boolean(contour.upperThreshold) || contour.upperThreshold === 0;
+ const validStrokeWidth =
+ Boolean(contour.strokeWidth) || contour.strokeWidth === 0;
+ const validColor =
+ typeof contour.color === 'object' &&
+ 'r' in contour.color &&
+ typeof contour.color.r === 'number' &&
+ 'g' in contour.color &&
+ typeof contour.color.g === 'number' &&
+ 'b' in contour.color &&
+ typeof contour.color.b === 'number' &&
+ 'a' in contour.color &&
+ typeof contour.color.a === 'number';
+
+ const errors = determineErrorMap(contour);
+ setValidationErrors(errors);
+
+ setIsComplete(
+ isIsoband
+ ? validLower && validUpper && validColor
+ : validLower && validColor && validStrokeWidth,
+ );
+ }, [contour, currentTab]);
+
+ const onTabChange = (activeKey: any) => {
+ setCurrentTab(activeKey);
+ };
+
+ const updateStrokeWidth = (value: number | string) => {
+ const newContour = { ...contour };
+ newContour.strokeWidth = value;
+ setContour(newContour);
+ };
+
+ const updateColor = (rgb: colorType) => {
+ const newContour = { ...contour };
+ newContour.color = { ...rgb, a: 100 };
+ setContour(newContour);
+ };
+
+ const updateLowerThreshold = (value: number | string) => {
+ const newContour = { ...contour };
+ newContour.lowerThreshold = value;
+ setContour(newContour);
+ };
+
+ const updateUpperThreshold = (value: number | string) => {
+ const newContour = { ...contour };
+ newContour.upperThreshold = value;
+ setContour(newContour);
+ };
+
+ const containsErrors = () => {
+ const keys = Object.keys(validationErrors);
+ return keys.some(key => validationErrors[key].length > 0);
+ };
+
+ const handleSave = () => {
+ if (isComplete && onSave) {
+ onSave(convertContourToNumeric(contour));
+ }
+ };
+
+ const isobandSection = () => (
+ <div className="isoline-popover-section">
+ <StyledRow>
+ <Col flex="1">
+ <ControlHeader
+ name="isoband-threshold-lower"
+ label="Lower Threshold"
+ description="The lower limit of the threshold range of the Isoband"
+ validationErrors={validationErrors.lowerThreshold}
+ hovered
+ />
+ <TextControl
+ value={contour.lowerThreshold}
+ onChange={updateLowerThreshold}
+ />
+ </Col>
+ <Col flex="1">
+ <ControlHeader
+ name="isoband-threshold-upper"
+ label="Upper Threshold"
+ description="The upper limit of the threshold range of the Isoband"
+ validationErrors={validationErrors.upperThreshold}
+ hovered
+ />
+ <TextControl
+ value={contour.upperThreshold}
+ onChange={updateUpperThreshold}
+ />
+ </Col>
+ </StyledRow>
+ <StyledRow>
+ <Col flex="1">
+ <ControlHeader
+ name="isoband-color"
+ label="Color"
+ description="The color of the isoband"
+ validationErrors={validationErrors.color}
+ hovered
+ />
+ <ColorPickerControl
+ value={typeof contour === 'object' && contour?.color}
+ onChange={updateColor}
+ />
+ </Col>
+ </StyledRow>
+ </div>
+ );
+
+ const isolineSection = () => (
+ <div className="isoline-popover-section">
+ <StyledRow>
+ <Col flex="1">
+ <ControlHeader
+ name="isoline-threshold"
+ label="Threshold"
+ description="Defines the value that determines the boundary between different regions or levels in the data "
+ validationErrors={validationErrors.lowerThreshold}
+ hovered
+ />
+ <TextControl
+ value={contour.lowerThreshold}
+ onChange={updateLowerThreshold}
+ />
+ </Col>
+ </StyledRow>
+ <StyledRow>
+ <Col flex="1">
+ <ControlHeader
+ name="isoline-stroke-width"
+ label="Stroke Width"
+ description="The width of the Isoline in pixels"
+ validationErrors={validationErrors.strokeWidth}
+ hovered
+ />
+ <TextControl
+ value={contour.strokeWidth}
+ onChange={updateStrokeWidth}
+ />
+ </Col>
+ <Col flex="1">
+ <ControlHeader
+ name="isoline-color"
+ label="Color"
+ description="The color of the isoline"
+ validationErrors={validationErrors.color}
+ hovered
+ />
+ <ColorPickerControl
+ value={typeof contour === 'object' && contour?.color}
+ onChange={updateColor}
+ />
+ </Col>
+ </StyledRow>
+ </div>
+ );
+
+ return (
+ <>
+ <Tabs
+ id="contour-edit-tabs"
+ onChange={onTabChange}
+ defaultActiveKey={getTabKey(initialValue)}
+ >
+ <Tabs.TabPane
+ className="adhoc-filter-edit-tab"
+ key={CONTOUR_TYPES.Isoline}
+ tab="Isoline"
Review Comment:
Translations for tab names too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297357934
##########
superset-frontend/src/explore/components/controls/ContourControl/index.tsx:
##########
@@ -0,0 +1,143 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState, useEffect } from 'react';
+import styled from '@emotion/styled';
+import DndSelectLabel from 'src/explore/components/controls/DndColumnSelectControl/DndSelectLabel';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import ContourOption from './ContourOption';
+import { contourType, ContourControlProps } from './types';
+
+const DEFAULT_CONTOURS: contourType[] = [
+ {
+ lowerThreshold: 4,
+ color: { r: 255, g: 0, b: 255, a: 100 },
+ strokeWidth: 1,
+ zIndex: 0,
+ },
+ {
+ lowerThreshold: 5,
+ color: { r: 0, g: 255, b: 0, a: 100 },
+ strokeWidth: 2,
+ zIndex: 1,
+ },
+ {
+ lowerThreshold: 6,
+ upperThreshold: 10,
+ color: { r: 0, g: 0, b: 255, a: 100 },
+ zIndex: 2,
+ },
+];
+
+const NewContourFormatPlaceholder = styled('div')`
+ position: relative;
+ width: calc(100% - 4px);
Review Comment:
theme grid units here too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Mattc1221 commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "Mattc1221 (via GitHub)" <gi...@apache.org>.
Mattc1221 commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1740268333
@kgabryje I'm sorry about the delay!
The last comment on the unfocusing issues should now be fixed. I had pulled out the tabs sections into functions within the component. This was causing them to get re-rendered on every change and thus lose focus. I've put them inside of the return statement of the surrounding component
https://github.com/apache/superset/assets/36670322/9e463eb0-bd39-4cef-b947-42b3ce932f57
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Add Deck.gl Contour Layer [superset]
Posted by "Mattc1221 (via GitHub)" <gi...@apache.org>.
Mattc1221 commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1752135285
@villebro @kgabryje @rusackas @justinpark @michael-s-molina I was wondering if one of you could take a final pass through this PR. All of the previous comments have been addressed and the PR should be ready to go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Mattc1221 commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "Mattc1221 (via GitHub)" <gi...@apache.org>.
Mattc1221 commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1597548978
> > @villebro @rusackas @kgabryje @justinpark @michael-s-molina
>
> >
>
> > I was wondering what an ETA on the timeline for review would be? Thanks again!
>
>
>
> Hi @Mattc1221 , sorry for the delay and thanks for the ping. I'll put this on my review list for today.
Thank you @villebro, I really appreciate it 😃
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1596553596
> @villebro @rusackas @kgabryje @justinpark @michael-s-molina
>
> I was wondering what an ETA on the timeline for review would be? Thanks again!
Hi @Mattc1221 , sorry for the delay and thanks for the ping. I'll put this on my review list for today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1682410380
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1682571262
Oh 1 more thing that I noticed - there isn't any tooltip with values when I hover over contours in the chart. Maybe it'd be worth to add it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297358898
##########
superset-frontend/src/explore/components/controls/ContourControl/index.tsx:
##########
@@ -0,0 +1,143 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState, useEffect } from 'react';
+import styled from '@emotion/styled';
+import DndSelectLabel from 'src/explore/components/controls/DndColumnSelectControl/DndSelectLabel';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import ContourOption from './ContourOption';
+import { contourType, ContourControlProps } from './types';
+
+const DEFAULT_CONTOURS: contourType[] = [
+ {
+ lowerThreshold: 4,
+ color: { r: 255, g: 0, b: 255, a: 100 },
+ strokeWidth: 1,
+ zIndex: 0,
+ },
+ {
+ lowerThreshold: 5,
+ color: { r: 0, g: 255, b: 0, a: 100 },
+ strokeWidth: 2,
+ zIndex: 1,
+ },
+ {
+ lowerThreshold: 6,
+ upperThreshold: 10,
+ color: { r: 0, g: 0, b: 255, a: 100 },
+ zIndex: 2,
+ },
+];
+
+const NewContourFormatPlaceholder = styled('div')`
+ position: relative;
+ width: calc(100% - 4px);
+ bottom: 16px;
+ left: 0;
+`;
+
+const ContourControl = ({ onChange, ...props }: ContourControlProps) => {
+ const [popoverVisible, setpopoverVisible] = useState(false);
+ const [contours, setContours] = useState<contourType[]>(
+ props?.value ? props?.value : DEFAULT_CONTOURS,
+ );
+
+ useEffect(() => {
+ // add z-index to contours
+ const newContours = contours.map((contour, index) => ({
+ ...contour,
+ zIndex: (index + 1) * 10,
+ }));
+ onChange?.(newContours);
+ }, [onChange, contours]);
+
+ const togglePopover = (visible: boolean) => {
+ setpopoverVisible(visible);
+ };
+
+ const handleClickGhostButton = () => {
+ togglePopover(true);
+ };
+
+ const saveContour = (contour: contourType) => {
+ setContours([...contours, contour]);
+ togglePopover(false);
+ };
+
+ const removeContour = (index: number) => {
+ const newContours = [...contours];
+ newContours.splice(index, 1);
+ setContours(newContours);
+ };
+
+ const onShiftContour = (hoverIndex: number, dragIndex: number) => {
+ const newContours = [...contours];
+ [newContours[hoverIndex], newContours[dragIndex]] = [
+ newContours[dragIndex],
+ newContours[hoverIndex],
+ ];
+ setContours(newContours);
+ };
+
+ const editContour = (contour: contourType, index: number) => {
+ const newContours = [...contours];
+ newContours[index] = contour;
+ setContours(newContours);
+ };
+
+ const valuesRenderer = () =>
+ contours.map((contour, index) => (
+ <ContourOption
+ key={index}
+ saveContour={(newContour: contourType) =>
+ editContour(newContour, index)
+ }
+ contour={contour}
+ index={index}
+ onClose={removeContour}
+ onShift={onShiftContour}
+ />
+ ));
+
+ const ghostButtonText = 'Click to add a contour';
Review Comment:
translation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1682416999
@kgabryje Ephemeral environment spinning up at http://35.92.143.13:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297376068
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/types.ts:
##########
@@ -32,6 +32,7 @@ export interface OptionProps {
isExtra?: boolean;
datasourceWarningMessage?: string;
canDelete?: boolean;
+ tooltipOverlay?: any;
Review Comment:
```suggestion
tooltipOverlay?: ReactNode;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297348299
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourPopoverControl.tsx:
##########
@@ -0,0 +1,311 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect } from 'react';
+import { Row, Col } from 'src/components';
+import Button from 'src/components/Button';
+import Tabs from 'src/components/Tabs';
+import { legacyValidateInteger, styled, t } from '@superset-ui/core';
+import ControlHeader from '../../ControlHeader';
+import TextControl from '../TextControl';
+import ColorPickerControl from '../ColorPickerControl';
+import {
+ ContourPopoverControlProps,
+ colorType,
+ contourType,
+ errorMapType,
+} from './types';
+
+enum CONTOUR_TYPES {
+ Isoline = 'ISOLINE',
+ Isoband = 'ISOBAND',
+}
+
+const ContourActionsContainer = styled.div`
+ margin-top: ${({ theme }) => theme.gridUnit * 2}px;
+`;
+
+const StyledRow = styled(Row)`
+ width: 100%;
+ gap: 10px;
Review Comment:
Theme grid unit here 🙏
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourPopoverControl.tsx:
##########
@@ -0,0 +1,311 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect } from 'react';
+import { Row, Col } from 'src/components';
+import Button from 'src/components/Button';
+import Tabs from 'src/components/Tabs';
+import { legacyValidateInteger, styled, t } from '@superset-ui/core';
+import ControlHeader from '../../ControlHeader';
+import TextControl from '../TextControl';
+import ColorPickerControl from '../ColorPickerControl';
+import {
+ ContourPopoverControlProps,
+ colorType,
+ contourType,
+ errorMapType,
+} from './types';
+
+enum CONTOUR_TYPES {
+ Isoline = 'ISOLINE',
+ Isoband = 'ISOBAND',
+}
+
+const ContourActionsContainer = styled.div`
+ margin-top: ${({ theme }) => theme.gridUnit * 2}px;
+`;
+
+const StyledRow = styled(Row)`
+ width: 100%;
+ gap: 10px;
Review Comment:
Theme grid unit here 🙏
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297347780
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourOption.tsx:
##########
@@ -0,0 +1,101 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled } from '@superset-ui/core';
+import { ContourOptionProps } from './types';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import OptionWrapper from '../DndColumnSelectControl/OptionWrapper';
+
+const StyledOptionWrapper = styled(OptionWrapper)`
+ max-width: 100%;
+ min-width: 100%;
+`;
+
+const StyledListItem = styled.li`
+ display: flex;
+ align-items: center;
+`;
+
+const ColorPatch = styled.div<{ formattedColor: string }>`
+ background-color: ${({ formattedColor }) => formattedColor};
+ height: 5px;
+ width: 5px;
+ margin: 0 5px;
+`;
+
+const ContourOption = ({
+ contour,
+ index,
+ saveContour,
+ onClose,
+ onShift,
+}: ContourOptionProps) => {
+ const { lowerThreshold, upperThreshold, color, strokeWidth } = contour;
+
+ const isIsoband = upperThreshold;
+
+ const formattedColor = color
+ ? `rgba(${color.r}, ${color.g}, ${color.b}, 1)`
+ : 'undefined';
+
+ const formatIsoline = (threshold: number, width: any) =>
+ `Threshold: ${threshold}, color: ${formattedColor}, stroke width: ${width}`;
+
+ const formatIsoband = (threshold: number[]) =>
+ `Threshold: [${threshold[0]}, ${threshold[1]}], color: ${formattedColor}`;
+
+ const displayString = isIsoband
+ ? formatIsoband([lowerThreshold || -1, upperThreshold])
+ : formatIsoline(lowerThreshold || -1, strokeWidth);
+
+ const overlay = (
+ <div className="contour-tooltip-overlay">
+ <StyledListItem>
+ Threshold:{' '}
+ {isIsoband
+ ? `[${lowerThreshold}, ${upperThreshold}]`
+ : `${lowerThreshold}`}
+ </StyledListItem>
+ <StyledListItem>
+ Color: <ColorPatch formattedColor={formattedColor} /> {formattedColor}
+ </StyledListItem>
+ {!isIsoband && (
+ <StyledListItem>Stroke Width: {strokeWidth}</StyledListItem>
+ )}
+ </div>
+ );
+
+ return (
+ <ContourPopoverTrigger saveContour={saveContour} value={contour}>
+ <StyledOptionWrapper
+ key={index}
Review Comment:
Is the key required here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297342623
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourOption.tsx:
##########
@@ -0,0 +1,101 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled } from '@superset-ui/core';
+import { ContourOptionProps } from './types';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import OptionWrapper from '../DndColumnSelectControl/OptionWrapper';
+
+const StyledOptionWrapper = styled(OptionWrapper)`
+ max-width: 100%;
+ min-width: 100%;
+`;
+
+const StyledListItem = styled.li`
+ display: flex;
+ align-items: center;
+`;
+
+const ColorPatch = styled.div<{ formattedColor: string }>`
+ background-color: ${({ formattedColor }) => formattedColor};
+ height: 5px;
Review Comment:
Could we use grid units from the theme here? Like `${({ theme }) => theme.gridUnit }px;`. 1 grid unit is 4px and we prefer using whole units, so it's going to be 4px instead of 5 but I guess that shouldn't make much difference here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Mattc1221 commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "Mattc1221 (via GitHub)" <gi...@apache.org>.
Mattc1221 commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1585792368
@kgabryje, @rusackas and @villebro
Since you reviewed https://github.com/apache/superset/pull/23551 I was wondering if you would take a look at this PR as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1680859675
@Mattc1221 @JohnDietrich-Pepper Thank you for ping and sorry for long delay - I'll review the PR by tomorrow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297353190
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourPopoverControl.tsx:
##########
@@ -0,0 +1,311 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect } from 'react';
+import { Row, Col } from 'src/components';
+import Button from 'src/components/Button';
+import Tabs from 'src/components/Tabs';
+import { legacyValidateInteger, styled, t } from '@superset-ui/core';
+import ControlHeader from '../../ControlHeader';
+import TextControl from '../TextControl';
+import ColorPickerControl from '../ColorPickerControl';
+import {
+ ContourPopoverControlProps,
+ colorType,
+ contourType,
+ errorMapType,
+} from './types';
+
+enum CONTOUR_TYPES {
+ Isoline = 'ISOLINE',
+ Isoband = 'ISOBAND',
+}
+
+const ContourActionsContainer = styled.div`
+ margin-top: ${({ theme }) => theme.gridUnit * 2}px;
+`;
+
+const StyledRow = styled(Row)`
+ width: 100%;
+ gap: 10px;
+`;
+
+const isIsoband = (contour: contourType) => {
+ if (Object.keys(contour).length < 4) {
+ return false;
+ }
+ return contour.upperThreshold && contour.lowerThreshold;
+};
+
+const getTabKey = (contour: contourType | undefined) =>
+ contour && isIsoband(contour) ? CONTOUR_TYPES.Isoband : CONTOUR_TYPES.Isoline;
+
+const determineErrorMap = (contour: contourType) => {
+ const type = getTabKey(contour);
+ const errorMap: errorMapType = {
+ lowerThreshold: [],
+ upperThreshold: [],
+ strokeWidth: [],
+ color: [],
+ };
+ if (type === CONTOUR_TYPES.Isoband) {
+ const upperThresholdError = legacyValidateInteger(contour.upperThreshold);
+ if (upperThresholdError) errorMap.upperThreshold.push(upperThresholdError);
+ }
+ const lowerThresholdError = legacyValidateInteger(contour.lowerThreshold);
+ const strokeWidthError = legacyValidateInteger(contour.strokeWidth);
+ if (lowerThresholdError) errorMap.lowerThreshold.push(lowerThresholdError);
+ if (strokeWidthError) errorMap.strokeWidth.push(strokeWidthError);
+ return errorMap;
+};
+
+const convertContourToNumeric = (contour: contourType) => {
+ const formattedContour = { ...contour };
+ const numericKeys = ['lowerThreshold', 'upperThreshold', 'strokeWidth'];
+ numericKeys.forEach(key => {
+ formattedContour[key] = Number(formattedContour[key]);
+ });
+ return formattedContour;
+};
+
+const DEFAULT_CONTOUR = {
+ lowerThreshold: undefined,
+ upperThreshold: undefined,
+ color: undefined,
+ strokeWidth: undefined,
+};
+
+const ContourPopoverControl = ({
+ value: initialValue,
+ onSave,
+ onClose,
+}: ContourPopoverControlProps) => {
+ const [currentTab, setCurrentTab] = useState(getTabKey(initialValue));
+ const [contour, setContour] = useState(initialValue || DEFAULT_CONTOUR);
+ const [validationErrors, setValidationErrors] = useState(
+ determineErrorMap(initialValue || DEFAULT_CONTOUR),
+ );
+ const [isComplete, setIsComplete] = useState(false);
+
+ useEffect(() => {
+ const isIsoband = currentTab === CONTOUR_TYPES.Isoband;
+ const validLower =
+ Boolean(contour.lowerThreshold) || contour.lowerThreshold === 0;
+ const validUpper =
+ Boolean(contour.upperThreshold) || contour.upperThreshold === 0;
+ const validStrokeWidth =
+ Boolean(contour.strokeWidth) || contour.strokeWidth === 0;
+ const validColor =
+ typeof contour.color === 'object' &&
+ 'r' in contour.color &&
+ typeof contour.color.r === 'number' &&
+ 'g' in contour.color &&
+ typeof contour.color.g === 'number' &&
+ 'b' in contour.color &&
+ typeof contour.color.b === 'number' &&
+ 'a' in contour.color &&
+ typeof contour.color.a === 'number';
+
+ const errors = determineErrorMap(contour);
+ setValidationErrors(errors);
+
+ setIsComplete(
+ isIsoband
+ ? validLower && validUpper && validColor
+ : validLower && validColor && validStrokeWidth,
+ );
+ }, [contour, currentTab]);
+
+ const onTabChange = (activeKey: any) => {
+ setCurrentTab(activeKey);
+ };
+
+ const updateStrokeWidth = (value: number | string) => {
+ const newContour = { ...contour };
+ newContour.strokeWidth = value;
+ setContour(newContour);
+ };
+
+ const updateColor = (rgb: colorType) => {
+ const newContour = { ...contour };
+ newContour.color = { ...rgb, a: 100 };
+ setContour(newContour);
+ };
+
+ const updateLowerThreshold = (value: number | string) => {
+ const newContour = { ...contour };
+ newContour.lowerThreshold = value;
+ setContour(newContour);
+ };
+
+ const updateUpperThreshold = (value: number | string) => {
+ const newContour = { ...contour };
+ newContour.upperThreshold = value;
+ setContour(newContour);
+ };
+
+ const containsErrors = () => {
+ const keys = Object.keys(validationErrors);
+ return keys.some(key => validationErrors[key].length > 0);
+ };
+
+ const handleSave = () => {
+ if (isComplete && onSave) {
+ onSave(convertContourToNumeric(contour));
+ }
+ };
+
+ const isobandSection = () => (
+ <div className="isoline-popover-section">
+ <StyledRow>
+ <Col flex="1">
+ <ControlHeader
+ name="isoband-threshold-lower"
+ label="Lower Threshold"
+ description="The lower limit of the threshold range of the Isoband"
+ validationErrors={validationErrors.lowerThreshold}
+ hovered
+ />
+ <TextControl
+ value={contour.lowerThreshold}
+ onChange={updateLowerThreshold}
+ />
+ </Col>
+ <Col flex="1">
+ <ControlHeader
+ name="isoband-threshold-upper"
+ label="Upper Threshold"
+ description="The upper limit of the threshold range of the Isoband"
+ validationErrors={validationErrors.upperThreshold}
+ hovered
+ />
+ <TextControl
+ value={contour.upperThreshold}
+ onChange={updateUpperThreshold}
+ />
+ </Col>
+ </StyledRow>
+ <StyledRow>
+ <Col flex="1">
+ <ControlHeader
+ name="isoband-color"
+ label="Color"
+ description="The color of the isoband"
+ validationErrors={validationErrors.color}
+ hovered
+ />
+ <ColorPickerControl
+ value={typeof contour === 'object' && contour?.color}
Review Comment:
In other components we're not checking for contour's type so I'm assuming it's not necessary here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297336598
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Contour/controlPanel.ts:
##########
@@ -0,0 +1,126 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ControlPanelConfig, sections } from '@superset-ui/chart-controls';
+import { t, validateNonEmpty } from '@superset-ui/core';
+import {
+ autozoom,
+ filterNulls,
+ jsColumns,
+ jsDataMutator,
+ jsOnclickHref,
+ jsTooltip,
+ mapboxStyle,
+ spatial,
+ viewport,
+} from '../../utilities/Shared_DeckGL';
+
+const config: ControlPanelConfig = {
+ controlPanelSections: [
+ sections.legacyRegularTime,
+ {
+ label: t('Query'),
+ expanded: true,
+ controlSetRows: [
+ [spatial],
+ ['row_limit'],
+ ['size'],
+ [filterNulls],
+ ['adhoc_filters'],
+ ],
+ },
+ {
+ label: t('Map'),
+ expanded: true,
+ controlSetRows: [
+ [mapboxStyle, viewport],
+ [autozoom],
+ [
+ {
+ name: 'cellSize',
+ config: {
+ type: 'TextControl',
+ label: t('Cell Size'),
+ default: 300,
+ isInt: true,
+ description: t('The size of each cell in meters'),
+ renderTrigger: true,
+ clearable: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'aggregation',
+ config: {
+ type: 'SelectControl',
+ label: t('Aggregation'),
+ description: t(
+ 'The function to use when aggregating points into groups',
+ ),
+ default: 'sum',
+ clearable: false,
+ renderTrigger: true,
+ choices: [
+ ['sum', t('sum')],
+ ['mean', t('mean')],
+ ],
+ },
+ },
+ ],
+ [
+ {
+ name: 'contours',
+ config: {
+ type: 'ContourControl',
+ label: t('Contours'),
+ renderTrigger: true,
+ description: t(
+ 'Define contour layers. Isolines represent a collection of line segments that ' +
+ 'serparate the area above and below a given threshold. Isobands represent a ' +
+ 'collection of polygons that fill the are containing values in a given ' +
+ 'threshold range.',
+ ),
+ },
+ },
+ ],
+ ],
+ },
+ {
+ label: t('Advanced'),
+ controlSetRows: [
+ [jsColumns],
+ [jsDataMutator],
+ [jsTooltip],
+ [jsOnclickHref],
+ ],
+ },
+ ],
+ controlOverrides: {
+ size: {
+ label: t('Weight'),
+ description: t("Metric used as a weight for the grid's coloring"),
+ validators: [validateNonEmpty],
+ },
+ },
+ formDataOverrides: formData => ({
Review Comment:
Shouldn't that be
```
formDataOverrides: formData => ({
...formData,
size: getStandardizedControls().shiftMetric(),
}),
```
?
It's used for retaining the control values when you switch the viz type, and it basically tells the control panel to prefill "size" control with the value of metric from the previous viz.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297357528
##########
superset-frontend/src/explore/components/controls/ContourControl/index.tsx:
##########
@@ -0,0 +1,143 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState, useEffect } from 'react';
+import styled from '@emotion/styled';
Review Comment:
PLease import styled from superset-ui/core
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1682487420
Thank you for the PR @Mattc1221! It looks fantastic - most of my comments are just nitpicks. I'd appreciate if you addressed my comment about the isoline/isoband popover - after that it looks good to go!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Mattc1221 commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "Mattc1221 (via GitHub)" <gi...@apache.org>.
Mattc1221 commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1616278414
@villebro Thanks for the review! I believe this PR is ready for re-review :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Mattc1221 commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "Mattc1221 (via GitHub)" <gi...@apache.org>.
Mattc1221 commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1692891200
Thanks @kgabryje for the detailed review, I believe I've addressed all of your comments!
> * I can change the contour type from isoline to isoband, but I can't do the opposite - when I edit an isoband, change the tab to isoline and type value, it only edits the lower threshold of isoband. I think user could expect that the contour becomes an isoline.
> * The popover should close after clicking save
> * Maybe we should have some kind of validation to ensure that upper threshold is larger than lower threshold?
You should now be able to change isobands to isolines, the Save button closes the form, I've added some validation messages and disabled saving when the lower threshold is greater than or equal to the upper threshold. The tooltip should now appear with the lat/long coordinates as well as the threshold for whatever contour the mouse is hovering.
https://github.com/apache/superset/assets/36670322/02e7cb41-183b-4932-95c8-1a51bcddfa1a
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Mattc1221 commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "Mattc1221 (via GitHub)" <gi...@apache.org>.
Mattc1221 commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1305203444
##########
superset-frontend/src/explore/components/controls/ContourControl/index.tsx:
##########
@@ -0,0 +1,143 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState, useEffect } from 'react';
+import styled from '@emotion/styled';
+import DndSelectLabel from 'src/explore/components/controls/DndColumnSelectControl/DndSelectLabel';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import ContourOption from './ContourOption';
+import { contourType, ContourControlProps } from './types';
+
+const DEFAULT_CONTOURS: contourType[] = [
+ {
+ lowerThreshold: 4,
+ color: { r: 255, g: 0, b: 255, a: 100 },
+ strokeWidth: 1,
+ zIndex: 0,
+ },
+ {
+ lowerThreshold: 5,
+ color: { r: 0, g: 255, b: 0, a: 100 },
+ strokeWidth: 2,
+ zIndex: 1,
+ },
+ {
+ lowerThreshold: 6,
+ upperThreshold: 10,
+ color: { r: 0, g: 0, b: 255, a: 100 },
+ zIndex: 2,
+ },
+];
+
+const NewContourFormatPlaceholder = styled('div')`
+ position: relative;
+ width: calc(100% - 4px);
+ bottom: 16px;
+ left: 0;
+`;
+
+const ContourControl = ({ onChange, ...props }: ContourControlProps) => {
+ const [popoverVisible, setpopoverVisible] = useState(false);
+ const [contours, setContours] = useState<contourType[]>(
+ props?.value ? props?.value : DEFAULT_CONTOURS,
+ );
+
+ useEffect(() => {
+ // add z-index to contours
+ const newContours = contours.map((contour, index) => ({
+ ...contour,
+ zIndex: (index + 1) * 10,
+ }));
+ onChange?.(newContours);
+ }, [onChange, contours]);
+
+ const togglePopover = (visible: boolean) => {
+ setpopoverVisible(visible);
+ };
+
+ const handleClickGhostButton = () => {
+ togglePopover(true);
+ };
+
+ const saveContour = (contour: contourType) => {
+ setContours([...contours, contour]);
+ togglePopover(false);
+ };
+
+ const removeContour = (index: number) => {
+ const newContours = [...contours];
+ newContours.splice(index, 1);
+ setContours(newContours);
+ };
+
+ const onShiftContour = (hoverIndex: number, dragIndex: number) => {
+ const newContours = [...contours];
+ [newContours[hoverIndex], newContours[dragIndex]] = [
+ newContours[dragIndex],
+ newContours[hoverIndex],
+ ];
+ setContours(newContours);
+ };
+
+ const editContour = (contour: contourType, index: number) => {
+ const newContours = [...contours];
+ newContours[index] = contour;
+ setContours(newContours);
+ };
+
+ const valuesRenderer = () =>
+ contours.map((contour, index) => (
+ <ContourOption
+ key={index}
+ saveContour={(newContour: contourType) =>
+ editContour(newContour, index)
+ }
+ contour={contour}
+ index={index}
+ onClose={removeContour}
+ onShift={onShiftContour}
+ />
+ ));
+
+ const ghostButtonText = 'Click to add a contour';
+
+ return (
+ <>
+ <DndSelectLabel
Review Comment:
Sounds good! I'm going to resolve this comment for now then, if you think it should be handled in this PR feel free to unresolve it and ping me!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Mattc1221 commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "Mattc1221 (via GitHub)" <gi...@apache.org>.
Mattc1221 commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1305110717
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourOption.tsx:
##########
@@ -0,0 +1,101 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled } from '@superset-ui/core';
+import { ContourOptionProps } from './types';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import OptionWrapper from '../DndColumnSelectControl/OptionWrapper';
+
+const StyledOptionWrapper = styled(OptionWrapper)`
+ max-width: 100%;
+ min-width: 100%;
+`;
+
+const StyledListItem = styled.li`
+ display: flex;
+ align-items: center;
+`;
+
+const ColorPatch = styled.div<{ formattedColor: string }>`
+ background-color: ${({ formattedColor }) => formattedColor};
+ height: 5px;
+ width: 5px;
+ margin: 0 5px;
+`;
+
+const ContourOption = ({
+ contour,
+ index,
+ saveContour,
+ onClose,
+ onShift,
+}: ContourOptionProps) => {
+ const { lowerThreshold, upperThreshold, color, strokeWidth } = contour;
+
+ const isIsoband = upperThreshold;
+
+ const formattedColor = color
+ ? `rgba(${color.r}, ${color.g}, ${color.b}, 1)`
+ : 'undefined';
+
+ const formatIsoline = (threshold: number, width: any) =>
+ `Threshold: ${threshold}, color: ${formattedColor}, stroke width: ${width}`;
+
+ const formatIsoband = (threshold: number[]) =>
+ `Threshold: [${threshold[0]}, ${threshold[1]}], color: ${formattedColor}`;
+
+ const displayString = isIsoband
+ ? formatIsoband([lowerThreshold || -1, upperThreshold])
+ : formatIsoline(lowerThreshold || -1, strokeWidth);
+
+ const overlay = (
+ <div className="contour-tooltip-overlay">
+ <StyledListItem>
+ Threshold:{' '}
Review Comment:
I've updated the overlay with translations (below). I'm not sure where to add translations on line 72. The result here is just numbers, do those need to be translated as well?
```.tsx
<div className="contour-tooltip-overlay">
<StyledListItem>
{t("Threshold: ")}
{isIsoband
? `[${lowerThreshold}, ${upperThreshold}]`
: `${lowerThreshold}`}
</StyledListItem>
<StyledListItem>
{t("Color: ")}<ColorPatch formattedColor={formattedColor} /> {t(`${formattedColor}`)}
</StyledListItem>
{!isIsoband && (
<StyledListItem>{t(`Stroke Width: ${strokeWidth}`)}</StyledListItem>
)}
</div>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1695520733
Thanks for updates @Mattc1221! 1 last thing - for some reason, the inputs unfocus after typing 1 character, which makes typing more than 1 digit difficult. I didn't notice that happening before the latest changes
https://github.com/apache/superset/assets/15073128/e91b218e-db16-41ce-97c6-e5a64c0045b2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Mattc1221 commented on pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "Mattc1221 (via GitHub)" <gi...@apache.org>.
Mattc1221 commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1657228703
Hello @villebro! I was just wondering what the status update is for this PR? All of the checks are passing and the requested changes have been made!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Add Deck.gl Contour Layer [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24154:
URL: https://github.com/apache/superset/pull/24154#issuecomment-1754798381
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Add Deck.gl Contour Layer [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje merged PR #24154:
URL: https://github.com/apache/superset/pull/24154
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297344628
##########
superset-frontend/src/explore/components/controls/ContourControl/ContourOption.tsx:
##########
@@ -0,0 +1,101 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership. The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { styled } from '@superset-ui/core';
+import { ContourOptionProps } from './types';
+import ContourPopoverTrigger from './ContourPopoverTrigger';
+import OptionWrapper from '../DndColumnSelectControl/OptionWrapper';
+
+const StyledOptionWrapper = styled(OptionWrapper)`
+ max-width: 100%;
+ min-width: 100%;
+`;
+
+const StyledListItem = styled.li`
+ display: flex;
+ align-items: center;
+`;
+
+const ColorPatch = styled.div<{ formattedColor: string }>`
+ background-color: ${({ formattedColor }) => formattedColor};
+ height: 5px;
+ width: 5px;
+ margin: 0 5px;
+`;
+
+const ContourOption = ({
+ contour,
+ index,
+ saveContour,
+ onClose,
+ onShift,
+}: ContourOptionProps) => {
+ const { lowerThreshold, upperThreshold, color, strokeWidth } = contour;
+
+ const isIsoband = upperThreshold;
+
+ const formattedColor = color
+ ? `rgba(${color.r}, ${color.g}, ${color.b}, 1)`
+ : 'undefined';
+
+ const formatIsoline = (threshold: number, width: any) =>
+ `Threshold: ${threshold}, color: ${formattedColor}, stroke width: ${width}`;
Review Comment:
Could you wrap that in translation (`t(...)`)? Same 2 lines below
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #24154: feat: Add Deck.gl Contour Layer
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24154:
URL: https://github.com/apache/superset/pull/24154#discussion_r1297449698
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Contour/controlPanel.ts:
##########
@@ -0,0 +1,126 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ControlPanelConfig, sections } from '@superset-ui/chart-controls';
+import { t, validateNonEmpty } from '@superset-ui/core';
+import {
+ autozoom,
+ filterNulls,
+ jsColumns,
+ jsDataMutator,
+ jsOnclickHref,
+ jsTooltip,
+ mapboxStyle,
+ spatial,
+ viewport,
+} from '../../utilities/Shared_DeckGL';
+
+const config: ControlPanelConfig = {
+ controlPanelSections: [
+ sections.legacyRegularTime,
+ {
+ label: t('Query'),
+ expanded: true,
+ controlSetRows: [
+ [spatial],
+ ['row_limit'],
+ ['size'],
+ [filterNulls],
+ ['adhoc_filters'],
+ ],
+ },
+ {
+ label: t('Map'),
+ expanded: true,
+ controlSetRows: [
+ [mapboxStyle, viewport],
+ [autozoom],
+ [
+ {
+ name: 'cellSize',
+ config: {
+ type: 'TextControl',
+ label: t('Cell Size'),
+ default: 300,
+ isInt: true,
+ description: t('The size of each cell in meters'),
+ renderTrigger: true,
+ clearable: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'aggregation',
+ config: {
+ type: 'SelectControl',
+ label: t('Aggregation'),
+ description: t(
+ 'The function to use when aggregating points into groups',
+ ),
+ default: 'sum',
+ clearable: false,
+ renderTrigger: true,
+ choices: [
+ ['sum', t('sum')],
+ ['mean', t('mean')],
Review Comment:
deck.gl also allows min and max, should we add them too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org