You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/09/23 07:29:31 UTC

[GitHub] [incubator-superset] kgabryje opened a new pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

kgabryje opened a new pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012


   ### SUMMARY
   The goal is to replace all uses of `reactable-arc` library with DataTable from superset-ui (which uses `react-table` under the hood) and keep the UI/UX mostly unchanged. The reason for that is that `reactable-arc` has been unsupported for 2 years now, uses deprecated lifecycle methods and is incompatible with Typescript.
   This PR replaces `reactable-arc` in `AlteredSliceTag` component.
   I also changed the logic of `AlteredSliceTag` to rely on rows rather than diffs in state, which caused some refactors in the unit tests.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   Before (the rows don't render - a bug perhaps?):
   
   ![image](https://user-images.githubusercontent.com/15073128/93980659-34fea900-fd7f-11ea-902b-a08450950c67.png)
   
   After:
   
   ![image](https://user-images.githubusercontent.com/15073128/93980546-0aaceb80-fd7f-11ea-9427-68e0236d96b5.png)
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] 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.

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] [incubator-superset] kgabryje commented on a change in pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012#discussion_r494916944



##########
File path: superset-frontend/src/components/AlteredSliceTag.jsx
##########
@@ -103,10 +138,7 @@ export default class AlteredSliceTag extends React.Component {
     if (value === null) {
       return 'null';
     }
-    if (
-      this.state.controlsMap[key] &&
-      this.state.controlsMap[key].type === 'AdhocFilterControl'
-    ) {
+    if (controlsMap[key] && controlsMap[key].type === 'AdhocFilterControl') {

Review comment:
       👍 




----------------------------------------------------------------
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.

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] [incubator-superset] kgabryje commented on a change in pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012#discussion_r494917912



##########
File path: superset-frontend/src/components/AlteredSliceTag.jsx
##########
@@ -65,7 +65,18 @@ export default class AlteredSliceTag extends React.Component {
       return;
     }
     const diffs = this.getDiffs(newProps);
-    this.setState({ diffs, hasDiffs: !isEmpty(diffs) });
+    this.setState(prevState => ({
+      rows: this.getRowsFromDiffs(diffs, prevState.controlsMap),
+      hasDiffs: !isEmpty(diffs),
+    }));
+  }
+
+  getRowsFromDiffs(diffs, controlsMap = this.state.controlsMap) {

Review comment:
       I agree, that test made little sense. I changed the logic so that we spy on `getRowsFromDiffs`  to check if it was called and if `state.rows` equals a mocked return value from `getRowsFromDiffs`.
   Is that what you meant?




----------------------------------------------------------------
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.

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] [incubator-superset] eschutho commented on a change in pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012#discussion_r494608199



##########
File path: superset-frontend/spec/javascripts/components/AlteredSliceTag_spec.jsx
##########
@@ -112,6 +113,28 @@ const expectedDiffs = {
   },
 };
 
+const expectedRows = [
+  {
+    control: 'Fake Filters',
+    before: 'a == hello',
+    after: 'b in [hello, my, name]',
+  },
+  {
+    control: 'Value bounds',
+    before: 'Min: 10, Max: 20',
+    after: 'Min: 15, Max: 16',
+  },
+  {
+    control: 'Fake Collection Control',
+    before: '{"1":"a","b":["6","g"]}',
+    after: '{"1":"a","b":[9,"15"],"t":"gggg"}',
+  },
+  { control: 'bool', before: 'false', after: 'true' },
+  { control: 'gucci', before: '1, 2, 3, 4', after: 'a, b, c, d' },
+  { control: 'never', before: 5, after: 10 },
+  { control: 'ever', before: '{"a":"b","c":"d"}', after: '{"x":"y","z":"z"}' },
+];
+

Review comment:
       not sure what our overall philosophy is on snapshots, but moving this to a snapshot could clean up this file a bit. Just a thought.

##########
File path: superset-frontend/src/components/AlteredSliceTag.jsx
##########
@@ -65,7 +65,18 @@ export default class AlteredSliceTag extends React.Component {
       return;
     }
     const diffs = this.getDiffs(newProps);
-    this.setState({ diffs, hasDiffs: !isEmpty(diffs) });
+    this.setState(prevState => ({
+      rows: this.getRowsFromDiffs(diffs, prevState.controlsMap),
+      hasDiffs: !isEmpty(diffs),
+    }));
+  }
+
+  getRowsFromDiffs(diffs, controlsMap = this.state.controlsMap) {

Review comment:
       is this default arg only used in the test? If so, it may be simpler to just use a static value in the test or just spy on this method and see if it got called. I'm not sure if testing against the value from this method adds much value in the test if the test is for checking that `UNSAFE_componentWillReceiveProps` called this action. If you are looking to see what the value is from this method, then making a separate unit test just for this method would be better, imo. 

##########
File path: superset-frontend/src/components/AlteredSliceTag.jsx
##########
@@ -103,10 +138,7 @@ export default class AlteredSliceTag extends React.Component {
     if (value === null) {
       return 'null';
     }
-    if (
-      this.state.controlsMap[key] &&
-      this.state.controlsMap[key].type === 'AdhocFilterControl'
-    ) {
+    if (controlsMap[key] && controlsMap[key].type === 'AdhocFilterControl') {

Review comment:
       you could use optional chaining here if you wanted to clean this up more. 




----------------------------------------------------------------
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.

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] [incubator-superset] rusackas merged pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
rusackas merged pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012


   


----------------------------------------------------------------
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.

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] [incubator-superset] kgabryje commented on pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012#issuecomment-698871919


   @eschutho Thanks for your comments, I did some changes. Can you take a look?


----------------------------------------------------------------
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.

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] [incubator-superset] kgabryje closed pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
kgabryje closed pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012


   


----------------------------------------------------------------
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.

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] [incubator-superset] eschutho commented on a change in pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012#discussion_r495299091



##########
File path: superset-frontend/src/components/AlteredSliceTag.jsx
##########
@@ -65,7 +65,18 @@ export default class AlteredSliceTag extends React.Component {
       return;
     }
     const diffs = this.getDiffs(newProps);
-    this.setState({ diffs, hasDiffs: !isEmpty(diffs) });
+    this.setState(prevState => ({
+      rows: this.getRowsFromDiffs(diffs, prevState.controlsMap),
+      hasDiffs: !isEmpty(diffs),
+    }));
+  }
+
+  getRowsFromDiffs(diffs, controlsMap = this.state.controlsMap) {

Review comment:
       yeah, that looks good!




----------------------------------------------------------------
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.

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] [incubator-superset] eschutho commented on a change in pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012#discussion_r495297892



##########
File path: superset-frontend/src/components/AlteredSliceTag.jsx
##########
@@ -90,11 +101,35 @@ export default class AlteredSliceTag extends React.Component {
     return diffs;
   }
 
+  sortData = ({ sortBy }) => {
+    if (this.state.rows.length > 0 && sortBy.length > 0) {
+      const { id, desc } = sortBy[0];
+      this.setState(({ rows }) => ({
+        rows: this.sortDataByColumn(rows, id, desc),
+      }));
+    }
+  };
+
+  sortDataByColumn(data, sortById, desc) {
+    return data.sort((row1, row2) => {
+      const rows = desc ? [row2, row1] : [row1, row2];
+      const firstVal = rows[0][sortById];
+      const secondVal = rows[1][sortById];
+      if (typeof firstVal === 'string' && typeof secondVal === 'string') {
+        return secondVal.localeCompare(firstVal);
+      }
+      if (typeof firstVal === 'undefined' || firstVal === null) {
+        return 1;
+      }
+      return -1;
+    });
+  }
+
   isEqualish(val1, val2) {
     return isEqual(alterForComparison(val1), alterForComparison(val2));
   }
 
-  formatValue(value, key) {
+  formatValue(value, key, controlsMap = this.state.controlsMap) {

Review comment:
       It looks like you can remove state from the default value now? Looks like you always have something to pass in. 




----------------------------------------------------------------
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.

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] [incubator-superset] eschutho commented on a change in pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012#discussion_r495298315



##########
File path: superset-frontend/src/components/AlteredSliceTag.jsx
##########
@@ -120,16 +152,10 @@ export default class AlteredSliceTag extends React.Component {
         })
         .join(', ');
     }
-    if (
-      this.state.controlsMap[key] &&
-      this.state.controlsMap[key].type === 'BoundsControl'
-    ) {
+    if (controlsMap[key] && controlsMap[key].type === 'BoundsControl') {
       return `Min: ${value[0]}, Max: ${value[1]}`;
     }
-    if (
-      this.state.controlsMap[key] &&
-      this.state.controlsMap[key].type === 'CollectionControl'
-    ) {
+    if (controlsMap[key] && controlsMap[key].type === 'CollectionControl') {

Review comment:
       you can use optional chaining here and line 155 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.

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] [incubator-superset] kgabryje commented on a change in pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012#discussion_r494916944



##########
File path: superset-frontend/src/components/AlteredSliceTag.jsx
##########
@@ -103,10 +138,7 @@ export default class AlteredSliceTag extends React.Component {
     if (value === null) {
       return 'null';
     }
-    if (
-      this.state.controlsMap[key] &&
-      this.state.controlsMap[key].type === 'AdhocFilterControl'
-    ) {
+    if (controlsMap[key] && controlsMap[key].type === 'AdhocFilterControl') {

Review comment:
       👍 

##########
File path: superset-frontend/spec/javascripts/components/AlteredSliceTag_spec.jsx
##########
@@ -112,6 +113,28 @@ const expectedDiffs = {
   },
 };
 
+const expectedRows = [
+  {
+    control: 'Fake Filters',
+    before: 'a == hello',
+    after: 'b in [hello, my, name]',
+  },
+  {
+    control: 'Value bounds',
+    before: 'Min: 10, Max: 20',
+    after: 'Min: 15, Max: 16',
+  },
+  {
+    control: 'Fake Collection Control',
+    before: '{"1":"a","b":["6","g"]}',
+    after: '{"1":"a","b":[9,"15"],"t":"gggg"}',
+  },
+  { control: 'bool', before: 'false', after: 'true' },
+  { control: 'gucci', before: '1, 2, 3, 4', after: 'a, b, c, d' },
+  { control: 'never', before: 5, after: 10 },
+  { control: 'ever', before: '{"a":"b","c":"d"}', after: '{"x":"y","z":"z"}' },
+];
+

Review comment:
       I moved all mocked values from AlteredSliceTag_spec.jsx to separate file

##########
File path: superset-frontend/src/components/AlteredSliceTag.jsx
##########
@@ -65,7 +65,18 @@ export default class AlteredSliceTag extends React.Component {
       return;
     }
     const diffs = this.getDiffs(newProps);
-    this.setState({ diffs, hasDiffs: !isEmpty(diffs) });
+    this.setState(prevState => ({
+      rows: this.getRowsFromDiffs(diffs, prevState.controlsMap),
+      hasDiffs: !isEmpty(diffs),
+    }));
+  }
+
+  getRowsFromDiffs(diffs, controlsMap = this.state.controlsMap) {

Review comment:
       I agree, that test made little sense. I changed the logic so that we spy on `getRowsFromDiffs`  to check if it was called and if `state.rows` equals a mocked return value from `getRowsFromDiffs`.
   Is that what you meant?




----------------------------------------------------------------
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.

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] [incubator-superset] eschutho commented on a change in pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012#discussion_r494608199



##########
File path: superset-frontend/spec/javascripts/components/AlteredSliceTag_spec.jsx
##########
@@ -112,6 +113,28 @@ const expectedDiffs = {
   },
 };
 
+const expectedRows = [
+  {
+    control: 'Fake Filters',
+    before: 'a == hello',
+    after: 'b in [hello, my, name]',
+  },
+  {
+    control: 'Value bounds',
+    before: 'Min: 10, Max: 20',
+    after: 'Min: 15, Max: 16',
+  },
+  {
+    control: 'Fake Collection Control',
+    before: '{"1":"a","b":["6","g"]}',
+    after: '{"1":"a","b":[9,"15"],"t":"gggg"}',
+  },
+  { control: 'bool', before: 'false', after: 'true' },
+  { control: 'gucci', before: '1, 2, 3, 4', after: 'a, b, c, d' },
+  { control: 'never', before: 5, after: 10 },
+  { control: 'ever', before: '{"a":"b","c":"d"}', after: '{"x":"y","z":"z"}' },
+];
+

Review comment:
       not sure what our overall philosophy is on snapshots, but moving this to a snapshot could clean up this file a bit. Just a thought.

##########
File path: superset-frontend/src/components/AlteredSliceTag.jsx
##########
@@ -65,7 +65,18 @@ export default class AlteredSliceTag extends React.Component {
       return;
     }
     const diffs = this.getDiffs(newProps);
-    this.setState({ diffs, hasDiffs: !isEmpty(diffs) });
+    this.setState(prevState => ({
+      rows: this.getRowsFromDiffs(diffs, prevState.controlsMap),
+      hasDiffs: !isEmpty(diffs),
+    }));
+  }
+
+  getRowsFromDiffs(diffs, controlsMap = this.state.controlsMap) {

Review comment:
       is this default arg only used in the test? If so, it may be simpler to just use a static value in the test or just spy on this method and see if it got called. I'm not sure if testing against the value from this method adds much value in the test if the test is for checking that `UNSAFE_componentWillReceiveProps` called this action. If you are looking to see what the value is from this method, then making a separate unit test just for this method would be better, imo. 

##########
File path: superset-frontend/src/components/AlteredSliceTag.jsx
##########
@@ -103,10 +138,7 @@ export default class AlteredSliceTag extends React.Component {
     if (value === null) {
       return 'null';
     }
-    if (
-      this.state.controlsMap[key] &&
-      this.state.controlsMap[key].type === 'AdhocFilterControl'
-    ) {
+    if (controlsMap[key] && controlsMap[key].type === 'AdhocFilterControl') {

Review comment:
       you could use optional chaining here if you wanted to clean this up more. 




----------------------------------------------------------------
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.

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] [incubator-superset] kgabryje commented on a change in pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012#discussion_r495825347



##########
File path: superset-frontend/src/components/AlteredSliceTag.jsx
##########
@@ -90,11 +101,35 @@ export default class AlteredSliceTag extends React.Component {
     return diffs;
   }
 
+  sortData = ({ sortBy }) => {
+    if (this.state.rows.length > 0 && sortBy.length > 0) {
+      const { id, desc } = sortBy[0];
+      this.setState(({ rows }) => ({
+        rows: this.sortDataByColumn(rows, id, desc),
+      }));
+    }
+  };
+
+  sortDataByColumn(data, sortById, desc) {
+    return data.sort((row1, row2) => {
+      const rows = desc ? [row2, row1] : [row1, row2];
+      const firstVal = rows[0][sortById];
+      const secondVal = rows[1][sortById];
+      if (typeof firstVal === 'string' && typeof secondVal === 'string') {
+        return secondVal.localeCompare(firstVal);
+      }
+      if (typeof firstVal === 'undefined' || firstVal === null) {
+        return 1;
+      }
+      return -1;
+    });
+  }
+
   isEqualish(val1, val2) {
     return isEqual(alterForComparison(val1), alterForComparison(val2));
   }
 
-  formatValue(value, key) {
+  formatValue(value, key, controlsMap = this.state.controlsMap) {

Review comment:
       Done

##########
File path: superset-frontend/src/components/AlteredSliceTag.jsx
##########
@@ -120,16 +152,10 @@ export default class AlteredSliceTag extends React.Component {
         })
         .join(', ');
     }
-    if (
-      this.state.controlsMap[key] &&
-      this.state.controlsMap[key].type === 'BoundsControl'
-    ) {
+    if (controlsMap[key] && controlsMap[key].type === 'BoundsControl') {
       return `Min: ${value[0]}, Max: ${value[1]}`;
     }
-    if (
-      this.state.controlsMap[key] &&
-      this.state.controlsMap[key].type === 'CollectionControl'
-    ) {
+    if (controlsMap[key] && controlsMap[key].type === 'CollectionControl') {

Review comment:
       Done




----------------------------------------------------------------
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.

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] [incubator-superset] kgabryje commented on pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012#issuecomment-698871919


   @eschutho Thanks for your comments, I did some changes. Can you take a look?


----------------------------------------------------------------
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.

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] [incubator-superset] kgabryje commented on a change in pull request #11012: refactor: Remove usages of reactable from AlteredSliceTag

Posted by GitBox <gi...@apache.org>.
kgabryje commented on a change in pull request #11012:
URL: https://github.com/apache/incubator-superset/pull/11012#discussion_r494917173



##########
File path: superset-frontend/spec/javascripts/components/AlteredSliceTag_spec.jsx
##########
@@ -112,6 +113,28 @@ const expectedDiffs = {
   },
 };
 
+const expectedRows = [
+  {
+    control: 'Fake Filters',
+    before: 'a == hello',
+    after: 'b in [hello, my, name]',
+  },
+  {
+    control: 'Value bounds',
+    before: 'Min: 10, Max: 20',
+    after: 'Min: 15, Max: 16',
+  },
+  {
+    control: 'Fake Collection Control',
+    before: '{"1":"a","b":["6","g"]}',
+    after: '{"1":"a","b":[9,"15"],"t":"gggg"}',
+  },
+  { control: 'bool', before: 'false', after: 'true' },
+  { control: 'gucci', before: '1, 2, 3, 4', after: 'a, b, c, d' },
+  { control: 'never', before: 5, after: 10 },
+  { control: 'ever', before: '{"a":"b","c":"d"}', after: '{"x":"y","z":"z"}' },
+];
+

Review comment:
       I moved all mocked values from AlteredSliceTag_spec.jsx to separate file




----------------------------------------------------------------
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.

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