You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "eschutho (via GitHub)" <gi...@apache.org> on 2023/12/01 01:10:48 UTC
[PR] chore: [bot] Run grit migration: Class to Functional - AlteredSliceTag [superset]
eschutho opened a new pull request, #26155:
URL: https://github.com/apache/superset/pull/26155
<!---
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 -->
Running a bot migration to convert class to functional component for AlteredSliceTag. Also update the tests to RTL.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
No changes
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
Tests were added. To manually test, you can find this component in the chart editor/explore
### 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
Re: [PR] chore: [bot] Run grit migration: Class to Functional - AlteredSliceTag [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #26155:
URL: https://github.com/apache/superset/pull/26155#issuecomment-1835251990
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/26155?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`630734b`)](https://app.codecov.io/gh/apache/superset/commit/630734b90896bcf9879200eb9eb287b370668f4e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 69.08% compared to head [(`7f0b782`)](https://app.codecov.io/gh/apache/superset/pull/26155?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 66.96%.
> Report is 31 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #26155 +/- ##
==========================================
- Coverage 69.08% 66.96% -2.12%
==========================================
Files 1940 1940
Lines 75870 75827 -43
Branches 8444 8425 -19
==========================================
- Hits 52415 50780 -1635
- Misses 21279 22875 +1596
+ Partials 2176 2172 -4
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/26155/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/26155/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.68% <ø> (ø)` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/26155/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.16% <ø> (?)` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/26155/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.24% <ø> (-0.02%)` | :arrow_down: |
| [presto](https://app.codecov.io/gh/apache/superset/pull/26155/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/26155/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.54% <ø> (-4.39%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/26155/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.91% <ø> (ø)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/26155/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/26155?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?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
Re: [PR] chore: [bot] Run grit migration: Class to Functional - AlteredSliceTag [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #26155:
URL: https://github.com/apache/superset/pull/26155#discussion_r1435411764
##########
superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.jsx:
##########
@@ -1,341 +1,169 @@
-/**
- * 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 from 'react';
-import { styledMount as mount } from 'spec/helpers/theming';
-import { getChartControlPanelRegistry } from '@superset-ui/core';
+import '@testing-library/jest-dom/extend-expect';
+import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import AlteredSliceTag, {
+ alterForComparison,
+ formatValueHandler,
+} from 'src/components/AlteredSliceTag';
+import { ThemeProvider, supersetTheme } from '@superset-ui/core';
+import { defaultProps, expectedRows } from './AlteredSliceTagMocks';
+
+const renderWithTheme = ui =>
+ render(<ThemeProvider theme={supersetTheme}>{ui}</ThemeProvider>);
Review Comment:
Ok, thanks!
--
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] chore: [bot] Run grit migration: Class to Functional - AlteredSliceTag [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #26155:
URL: https://github.com/apache/superset/pull/26155#discussion_r1434976736
##########
superset-frontend/src/components/AlteredSliceTag/AlteredSliceTag.test.jsx:
##########
@@ -1,341 +1,169 @@
-/**
- * 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 from 'react';
-import { styledMount as mount } from 'spec/helpers/theming';
-import { getChartControlPanelRegistry } from '@superset-ui/core';
+import '@testing-library/jest-dom/extend-expect';
+import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import AlteredSliceTag, {
+ alterForComparison,
+ formatValueHandler,
+} from 'src/components/AlteredSliceTag';
+import { ThemeProvider, supersetTheme } from '@superset-ui/core';
+import { defaultProps, expectedRows } from './AlteredSliceTagMocks';
+
+const renderWithTheme = ui =>
+ render(<ThemeProvider theme={supersetTheme}>{ui}</ThemeProvider>);
Review Comment:
If you import `render` from `spec/helpers/testing-library`, then you don't need a `ThemeProvider` wrapper
--
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] chore: [bot] Run grit migration: Class to Functional - AlteredSliceTag [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #26155:
URL: https://github.com/apache/superset/pull/26155#discussion_r1434994671
##########
superset-frontend/src/components/AlteredSliceTag/index.jsx:
##########
@@ -60,39 +61,73 @@ function alterForComparison(value) {
}
}
return value;
-}
-
-export default class AlteredSliceTag extends React.Component {
- constructor(props) {
- super(props);
- const diffs = this.getDiffs(props);
- const controlsMap = getControlsForVizType(this.props.origFormData.viz_type);
- const rows = this.getRowsFromDiffs(diffs, controlsMap);
+};
- this.state = { rows, hasDiffs: !isEmpty(diffs), controlsMap };
+export const formatValueHandler = (value, key, controlsMap) => {
+ // Format display value based on the control type
+ // or the value type
+ if (value === undefined) {
+ return 'N/A';
}
-
- UNSAFE_componentWillReceiveProps(newProps) {
- // Update differences if need be
- if (isEqual(this.props, newProps)) {
- return;
+ if (value === null) {
+ return 'null';
+ }
+ if (controlsMap[key]?.type === 'AdhocFilterControl') {
+ if (!value.length) {
+ return '[]';
}
- const diffs = this.getDiffs(newProps);
- this.setState(prevState => ({
- rows: this.getRowsFromDiffs(diffs, prevState.controlsMap),
- hasDiffs: !isEmpty(diffs),
- }));
+ return value
+ .map(v => {
+ const filterVal =
+ v.comparator && v.comparator.constructor === Array
+ ? `[${v.comparator.join(', ')}]`
+ : v.comparator;
+ return `${v.subject} ${v.operator} ${filterVal}`;
+ })
+ .join(', ');
}
-
- getRowsFromDiffs(diffs, controlsMap) {
- return Object.entries(diffs).map(([key, diff]) => ({
- control: (controlsMap[key] && controlsMap[key].label) || key,
- before: this.formatValue(diff.before, key, controlsMap),
- after: this.formatValue(diff.after, key, controlsMap),
- }));
+ if (controlsMap[key]?.type === 'BoundsControl') {
+ return `Min: ${value[0]}, Max: ${value[1]}`;
+ }
+ if (controlsMap[key]?.type === 'CollectionControl') {
+ return value.map(v => safeStringify(v)).join(', ');
+ }
+ if (
+ controlsMap[key]?.type === 'MetricsControl' &&
+ value.constructor === Array
+ ) {
+ const formattedValue = value.map(v => v?.label ?? v);
+ return formattedValue.length ? formattedValue.join(', ') : '[]';
+ }
+ if (typeof value === 'boolean') {
+ return value ? 'true' : 'false';
+ }
+ if (value.constructor === Array) {
+ const formattedValue = value.map(v => v?.label ?? v);
+ return formattedValue.length ? formattedValue.join(', ') : '[]';
+ }
+ if (typeof value === 'string' || typeof value === 'number') {
+ return value;
}
+ return safeStringify(value);
+};
+
+export const getRowsFromDiffs = (diffs, controlsMap) =>
+ Object.entries(diffs).map(([key, diff]) => ({
+ control: (controlsMap[key] && controlsMap[key].label) || key,
+ before: formatValueHandler(diff.before, key, controlsMap),
+ after: formatValueHandler(diff.after, key, controlsMap),
+ }));
+
+export const isEqualish = (val1, val2) =>
+ isEqual(alterForComparison(val1), alterForComparison(val2));
- getDiffs(props) {
+const AlteredSliceTag = props => {
+ const prevProps = useRef({});
Review Comment:
Can we use our hook `usePrevious` instead?
--
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] chore: [bot] Run grit migration: Class to Functional - AlteredSliceTag [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #26155:
URL: https://github.com/apache/superset/pull/26155#discussion_r1435005633
##########
superset-frontend/src/components/AlteredSliceTag/index.jsx:
##########
@@ -107,67 +142,38 @@ export default class AlteredSliceTag extends React.Component {
if (['filters', 'having', 'where'].includes(fdKey)) {
return;
}
- if (!this.isEqualish(ofd[fdKey], cfd[fdKey])) {
+ if (!isEqualish(ofd[fdKey], cfd[fdKey])) {
diffs[fdKey] = { before: ofd[fdKey], after: cfd[fdKey] };
}
});
return diffs;
- }
-
- isEqualish(val1, val2) {
- return isEqual(alterForComparison(val1), alterForComparison(val2));
- }
-
- formatValue(value, key, controlsMap) {
- // Format display value based on the control type
- // or the value type
- if (value === undefined) {
- return 'N/A';
- }
- if (value === null) {
- return 'null';
- }
- if (controlsMap[key]?.type === 'AdhocFilterControl') {
- if (!value.length) {
- return '[]';
+ }, []);
+
+ useEffect(() => {
+ const diffs = getDiffs(props);
+ const controlsMap = getControlsForVizType(props.origFormData?.viz_type);
+ const rows = getRowsFromDiffs(diffs, controlsMap);
+ const hasDiffs = !isEmpty(diffs);
+ setRows(rows);
+ setHasDiffs(hasDiffs);
+ }, []);
+
+ useEffect(() => {
+ const diffs = getDiffs(props);
+
+ const updateStateWithDiffs = newProps => {
+ if (isEqual(prevProps.current, newProps)) {
+ return;
}
- return value
- .map(v => {
- const filterVal =
- v.comparator && v.comparator.constructor === Array
- ? `[${v.comparator.join(', ')}]`
- : v.comparator;
- return `${v.subject} ${v.operator} ${filterVal}`;
- })
- .join(', ');
- }
- if (controlsMap[key]?.type === 'BoundsControl') {
- return `Min: ${value[0]}, Max: ${value[1]}`;
- }
- if (controlsMap[key]?.type === 'CollectionControl') {
- return value.map(v => safeStringify(v)).join(', ');
- }
- if (
- controlsMap[key]?.type === 'MetricsControl' &&
- value.constructor === Array
- ) {
- const formattedValue = value.map(v => v?.label ?? v);
- return formattedValue.length ? formattedValue.join(', ') : '[]';
- }
- if (typeof value === 'boolean') {
- return value ? 'true' : 'false';
- }
- if (value.constructor === Array) {
- const formattedValue = value.map(v => v?.label ?? v);
- return formattedValue.length ? formattedValue.join(', ') : '[]';
- }
- if (typeof value === 'string' || typeof value === 'number') {
- return value;
- }
- return safeStringify(value);
- }
+ setRows(prevRows => getRowsFromDiffs(diffs, prevRows));
+ setHasDiffs(!isEmpty(diffs));
+ };
+
+ updateStateWithDiffs(props);
+ prevProps.current = props;
+ }, [getDiffs, props]);
- renderModalBody() {
+ const renderModalBodyHandler = useCallback(() => {
Review Comment:
nit: I wonder if `Handler` suffix makes sense here? Since it doesn't really "handle" anything, and really it could be a value instead of a function
##########
superset-frontend/src/components/AlteredSliceTag/index.jsx:
##########
@@ -188,39 +194,39 @@ export default class AlteredSliceTag extends React.Component {
return (
<TableView
columns={columns}
- data={this.state.rows}
+ data={rows}
pageSize={50}
className="table-condensed"
columnsForWrapText={columnsForWrapText}
/>
);
- }
+ }, [rows]);
- renderTriggerNode() {
- return (
+ const renderTriggerNodeHandler = useCallback(
Review Comment:
Same as above
--
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] chore: [bot] Run grit migration: Class to Functional - AlteredSliceTag [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho closed pull request #26155: chore: [bot] Run grit migration: Class to Functional - AlteredSliceTag
URL: https://github.com/apache/superset/pull/26155
--
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] chore: [bot] Run grit migration: Class to Functional - AlteredSliceTag [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #26155:
URL: https://github.com/apache/superset/pull/26155#discussion_r1434995476
##########
superset-frontend/src/components/AlteredSliceTag/index.jsx:
##########
@@ -60,39 +61,73 @@ function alterForComparison(value) {
}
}
return value;
-}
-
-export default class AlteredSliceTag extends React.Component {
- constructor(props) {
- super(props);
- const diffs = this.getDiffs(props);
- const controlsMap = getControlsForVizType(this.props.origFormData.viz_type);
- const rows = this.getRowsFromDiffs(diffs, controlsMap);
+};
- this.state = { rows, hasDiffs: !isEmpty(diffs), controlsMap };
+export const formatValueHandler = (value, key, controlsMap) => {
+ // Format display value based on the control type
+ // or the value type
+ if (value === undefined) {
+ return 'N/A';
}
-
- UNSAFE_componentWillReceiveProps(newProps) {
- // Update differences if need be
- if (isEqual(this.props, newProps)) {
- return;
+ if (value === null) {
+ return 'null';
+ }
+ if (controlsMap[key]?.type === 'AdhocFilterControl') {
+ if (!value.length) {
+ return '[]';
}
- const diffs = this.getDiffs(newProps);
- this.setState(prevState => ({
- rows: this.getRowsFromDiffs(diffs, prevState.controlsMap),
- hasDiffs: !isEmpty(diffs),
- }));
+ return value
+ .map(v => {
+ const filterVal =
+ v.comparator && v.comparator.constructor === Array
+ ? `[${v.comparator.join(', ')}]`
+ : v.comparator;
+ return `${v.subject} ${v.operator} ${filterVal}`;
+ })
+ .join(', ');
}
-
- getRowsFromDiffs(diffs, controlsMap) {
- return Object.entries(diffs).map(([key, diff]) => ({
- control: (controlsMap[key] && controlsMap[key].label) || key,
- before: this.formatValue(diff.before, key, controlsMap),
- after: this.formatValue(diff.after, key, controlsMap),
- }));
+ if (controlsMap[key]?.type === 'BoundsControl') {
+ return `Min: ${value[0]}, Max: ${value[1]}`;
+ }
+ if (controlsMap[key]?.type === 'CollectionControl') {
+ return value.map(v => safeStringify(v)).join(', ');
+ }
+ if (
+ controlsMap[key]?.type === 'MetricsControl' &&
+ value.constructor === Array
+ ) {
+ const formattedValue = value.map(v => v?.label ?? v);
+ return formattedValue.length ? formattedValue.join(', ') : '[]';
+ }
+ if (typeof value === 'boolean') {
+ return value ? 'true' : 'false';
+ }
+ if (value.constructor === Array) {
+ const formattedValue = value.map(v => v?.label ?? v);
+ return formattedValue.length ? formattedValue.join(', ') : '[]';
+ }
+ if (typeof value === 'string' || typeof value === 'number') {
+ return value;
}
+ return safeStringify(value);
+};
+
+export const getRowsFromDiffs = (diffs, controlsMap) =>
+ Object.entries(diffs).map(([key, diff]) => ({
+ control: (controlsMap[key] && controlsMap[key].label) || key,
+ before: formatValueHandler(diff.before, key, controlsMap),
+ after: formatValueHandler(diff.after, key, controlsMap),
+ }));
+
+export const isEqualish = (val1, val2) =>
+ isEqual(alterForComparison(val1), alterForComparison(val2));
- getDiffs(props) {
+const AlteredSliceTag = props => {
+ const prevProps = useRef({});
+ const [rows, setRows] = useState([]);
+ const [hasDiffs, setHasDiffs] = useState(false);
+
+ const getDiffs = useCallback(props => {
Review Comment:
We can skip `props` argument since this function already has access to props
--
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] chore: [bot] Run grit migration: Class to Functional - AlteredSliceTag [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #26155:
URL: https://github.com/apache/superset/pull/26155#discussion_r1434995729
##########
superset-frontend/src/components/AlteredSliceTag/index.jsx:
##########
@@ -107,67 +142,38 @@ export default class AlteredSliceTag extends React.Component {
if (['filters', 'having', 'where'].includes(fdKey)) {
return;
}
- if (!this.isEqualish(ofd[fdKey], cfd[fdKey])) {
+ if (!isEqualish(ofd[fdKey], cfd[fdKey])) {
diffs[fdKey] = { before: ofd[fdKey], after: cfd[fdKey] };
}
});
return diffs;
- }
-
- isEqualish(val1, val2) {
- return isEqual(alterForComparison(val1), alterForComparison(val2));
- }
-
- formatValue(value, key, controlsMap) {
- // Format display value based on the control type
- // or the value type
- if (value === undefined) {
- return 'N/A';
- }
- if (value === null) {
- return 'null';
- }
- if (controlsMap[key]?.type === 'AdhocFilterControl') {
- if (!value.length) {
- return '[]';
+ }, []);
Review Comment:
We're missing dependencies in deps array
##########
superset-frontend/src/components/AlteredSliceTag/index.jsx:
##########
@@ -107,67 +142,38 @@ export default class AlteredSliceTag extends React.Component {
if (['filters', 'having', 'where'].includes(fdKey)) {
return;
}
- if (!this.isEqualish(ofd[fdKey], cfd[fdKey])) {
+ if (!isEqualish(ofd[fdKey], cfd[fdKey])) {
diffs[fdKey] = { before: ofd[fdKey], after: cfd[fdKey] };
}
});
return diffs;
- }
-
- isEqualish(val1, val2) {
- return isEqual(alterForComparison(val1), alterForComparison(val2));
- }
-
- formatValue(value, key, controlsMap) {
- // Format display value based on the control type
- // or the value type
- if (value === undefined) {
- return 'N/A';
- }
- if (value === null) {
- return 'null';
- }
- if (controlsMap[key]?.type === 'AdhocFilterControl') {
- if (!value.length) {
- return '[]';
+ }, []);
+
+ useEffect(() => {
+ const diffs = getDiffs(props);
+ const controlsMap = getControlsForVizType(props.origFormData?.viz_type);
+ const rows = getRowsFromDiffs(diffs, controlsMap);
+ const hasDiffs = !isEmpty(diffs);
+ setRows(rows);
+ setHasDiffs(hasDiffs);
+ }, []);
Review Comment:
Missing dependencies
--
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] chore: [bot] Run grit migration: Class to Functional - AlteredSliceTag [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #26155:
URL: https://github.com/apache/superset/pull/26155#discussion_r1435003109
##########
superset-frontend/src/components/AlteredSliceTag/index.jsx:
##########
@@ -107,67 +142,38 @@ export default class AlteredSliceTag extends React.Component {
if (['filters', 'having', 'where'].includes(fdKey)) {
return;
}
- if (!this.isEqualish(ofd[fdKey], cfd[fdKey])) {
+ if (!isEqualish(ofd[fdKey], cfd[fdKey])) {
diffs[fdKey] = { before: ofd[fdKey], after: cfd[fdKey] };
}
});
return diffs;
- }
-
- isEqualish(val1, val2) {
- return isEqual(alterForComparison(val1), alterForComparison(val2));
- }
-
- formatValue(value, key, controlsMap) {
- // Format display value based on the control type
- // or the value type
- if (value === undefined) {
- return 'N/A';
- }
- if (value === null) {
- return 'null';
- }
- if (controlsMap[key]?.type === 'AdhocFilterControl') {
- if (!value.length) {
- return '[]';
+ }, []);
+
+ useEffect(() => {
+ const diffs = getDiffs(props);
+ const controlsMap = getControlsForVizType(props.origFormData?.viz_type);
+ const rows = getRowsFromDiffs(diffs, controlsMap);
+ const hasDiffs = !isEmpty(diffs);
+ setRows(rows);
Review Comment:
nit: `const rows` and `const hasDiffs` are a bit redundant, we could do `setRows(getRowsFromDiffs(diffs, controlsMap)` and `setHasDiffs(!isEmpty(diffs))` and save 2 lines
--
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] chore: [bot] Run grit migration: Class to Functional - AlteredSliceTag [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #26155:
URL: https://github.com/apache/superset/pull/26155#discussion_r1451055511
##########
superset-frontend/src/components/AlteredSliceTag/index.jsx:
##########
@@ -107,67 +142,38 @@ export default class AlteredSliceTag extends React.Component {
if (['filters', 'having', 'where'].includes(fdKey)) {
return;
}
- if (!this.isEqualish(ofd[fdKey], cfd[fdKey])) {
+ if (!isEqualish(ofd[fdKey], cfd[fdKey])) {
diffs[fdKey] = { before: ofd[fdKey], after: cfd[fdKey] };
}
});
return diffs;
- }
-
- isEqualish(val1, val2) {
- return isEqual(alterForComparison(val1), alterForComparison(val2));
- }
-
- formatValue(value, key, controlsMap) {
- // Format display value based on the control type
- // or the value type
- if (value === undefined) {
- return 'N/A';
- }
- if (value === null) {
- return 'null';
- }
- if (controlsMap[key]?.type === 'AdhocFilterControl') {
- if (!value.length) {
- return '[]';
+ }, []);
+
+ useEffect(() => {
+ const diffs = getDiffs(props);
+ const controlsMap = getControlsForVizType(props.origFormData?.viz_type);
+ const rows = getRowsFromDiffs(diffs, controlsMap);
+ const hasDiffs = !isEmpty(diffs);
+ setRows(rows);
+ setHasDiffs(hasDiffs);
+ }, []);
+
+ useEffect(() => {
+ const diffs = getDiffs(props);
+
+ const updateStateWithDiffs = newProps => {
+ if (isEqual(prevProps.current, newProps)) {
+ return;
}
- return value
- .map(v => {
- const filterVal =
- v.comparator && v.comparator.constructor === Array
- ? `[${v.comparator.join(', ')}]`
- : v.comparator;
- return `${v.subject} ${v.operator} ${filterVal}`;
- })
- .join(', ');
- }
- if (controlsMap[key]?.type === 'BoundsControl') {
- return `Min: ${value[0]}, Max: ${value[1]}`;
- }
- if (controlsMap[key]?.type === 'CollectionControl') {
- return value.map(v => safeStringify(v)).join(', ');
- }
- if (
- controlsMap[key]?.type === 'MetricsControl' &&
- value.constructor === Array
- ) {
- const formattedValue = value.map(v => v?.label ?? v);
- return formattedValue.length ? formattedValue.join(', ') : '[]';
- }
- if (typeof value === 'boolean') {
- return value ? 'true' : 'false';
- }
- if (value.constructor === Array) {
- const formattedValue = value.map(v => v?.label ?? v);
- return formattedValue.length ? formattedValue.join(', ') : '[]';
- }
- if (typeof value === 'string' || typeof value === 'number') {
- return value;
- }
- return safeStringify(value);
- }
+ setRows(prevRows => getRowsFromDiffs(diffs, prevRows));
+ setHasDiffs(!isEmpty(diffs));
+ };
+
+ updateStateWithDiffs(props);
+ prevProps.current = props;
+ }, [getDiffs, props]);
- renderModalBody() {
+ const renderModalBodyHandler = useCallback(() => {
Review Comment:
it's a default suffix for grit, but we can see about changing it. I updated it manually 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