refactor: Convert TableElement to TypeScript by corbinrobb · Pull Request #14978 · apache/superset

corbinrobb · 2021-06-04T02:18:03Z

SUMMARY

Converted TableElement to TypeScript.

Required me to move the expand icon with tooltip off the Collapse.Panel component on to the Collapse parent component where it was supposed to be. Allowed me to remove most of the inline styling in the expand icon because the styles are now being inherited correctly.

Made code more concise and readable wherever I could.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Should look and behave the same

TESTING INSTRUCTIONS

To test TableElement navigate to the route superset/sqllab
On the left towards the bottom there should be a drop-down saying Select table or type table name
Use the drop-down and select some tables and below the drop-down, some TableElement components should appear
Check them out

ADDITIONAL INFORMATION

Has associated issue:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

hughhhh · 2021-06-04T18:56:42Z

/testenv up

github-actions · 2021-06-04T18:59:08Z

@hughhhh Container image not yet published for this PR. Please try again when build is complete.

github-actions · 2021-06-04T18:59:09Z

@hughhhh Ephemeral environment creation failed. Please check the Actions logs for details.

codecov · 2021-06-04T19:35:54Z

Codecov Report

Merging #14978 (80ca81a) into master (004a6d9) will decrease coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #14978      +/-   ##
==========================================
- Coverage   77.61%   77.45%   -0.17%     
==========================================
  Files         965      969       +4     
  Lines       49503    49944     +441     
  Branches     6259     6423     +164     
==========================================
+ Hits        38422    38682     +260     
- Misses      10881    11057     +176     
- Partials      200      205       +5

Flag	Coverage Δ
javascript	`72.23% <100.00%> (-0.26%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...rontend/src/SqlLab/components/SqlEditorLeftBar.jsx	`56.06% <100.00%> (+2.83%)`	⬆️
...et-frontend/src/SqlLab/components/TableElement.tsx	`92.00% <100.00%> (ø)`
...trols/DateFilterControl/components/CommonFrame.tsx	`41.66% <0.00%> (-50.00%)`	⬇️
...et-frontend/src/dashboard/actions/nativeFilters.ts	`50.84% <0.00%> (-30.01%)`	⬇️
...nd/src/dashboard/components/nativeFilters/utils.ts	`56.25% <0.00%> (-30.00%)`	⬇️
...nents/nativeFilters/FilterBar/FilterSets/index.tsx	`47.00% <0.00%> (-30.00%)`	⬇️
...nd/src/dashboard/components/nativeFilters/state.ts	`75.00% <0.00%> (-25.00%)`	⬇️
...t-frontend/src/dashboard/reducers/nativeFilters.ts	`60.00% <0.00%> (-24.62%)`	⬇️
...components/DashboardBuilder/DashboardContainer.tsx	`82.97% <0.00%> (-17.03%)`	⬇️
...nts/controls/DateFilterControl/DateFilterLabel.tsx	`58.47% <0.00%> (-15.26%)`	⬇️
... and 87 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 004a6d9...80ca81a. Read the comment docs.

AAfghahi

Looks good! Just some questions.

AAfghahi · 2021-06-10T14:39:55Z

  };
  isActive: boolean;
-  key: string | number;
+  panelKey: string | number;


oh that's so weird that it can be a string or a number. Wild.

AAfghahi · 2021-06-10T14:44:49Z

 `;

-const TableElement = props => {
+const TableElement = ({ table, actions, key, ...props }: TableElementProps) => {


it still bothers me that I don't know all of the props being passed in here. But I agree that this is much more elegant.

AAfghahi · 2021-06-10T15:14:59Z

  return (
    <Collapse.Panel
      {...props}
+      key={key}


What do you get when console.log key? I am looking at the old component and it looks really weird.

but key is specifically being put in.
https://github.com/apache/superset/blob/master/superset-frontend/src/SqlLab/components/SqlEditorLeftBar.jsx#L191

Also, where is this panelKey thing coming from!

I pulled your branch and console logged key:

AAfghahi · 2021-06-10T15:15:39Z

+    removeDataPreview: (table: Table) => void;
+    removeTable: (table: Table) => void;
+  };
+  key: string | number;


This looks like it might just be a string? Are there places where it is used as a number? (though it is odd that it is a string, since a key should be a number. . .)

… weirdness

eschutho · 2021-06-14T22:26:07Z


+  renderExpandIconWithTooltip = ({ isActive }) => (
+    <IconTooltip
+      style={{ transform: 'rotate(90deg)' }}


can we use the css prop here and below instead of style? It's a pattern we've been migrating to.

eschutho · 2021-06-14T22:36:01Z

        );
      }
-      let latest = Object.entries(table.partitions?.latest || []).map(
+      const latest = Object.entries(table.partitions?.latest || []).map(


small nit, but if we're not going to use latest again, can we just define it once as

const latest = Object.entries(table.partitions?.latest || []).map( ([key, value]) => `${key}=${value}`, ).join('/');

eschutho · 2021-06-14T22:40:48Z

+// These props are not defined in their TypeScript type for Panel though because
+// this logic is happening in the Collapse component itself. We have gotten around the TypeScript
+// errors by using the rest and spread operators to pass the necessary extra props to the Panels.
+


I think this comment makes sense, but the pattern below doesn't look confusing. I think it's ok to remove the comment.

Sounds good! I am removing the comment and adding the two other changes. I will push them all up momentarily.

…ning to make more concise

eschutho

Looks great @corbinrobb!

eschutho · 2021-06-14T23:23:20Z

@AAfghahi @corbinrobb would you like me to merge when this passes tests?

eschutho · 2021-06-15T00:06:22Z

/testenv up

corbinrobb · 2021-06-15T00:21:19Z

Thanks! @eschutho I am happy with merging it if you and @AAfghahi are!

eschutho · 2021-06-15T15:59:35Z

/testenv up

eschutho · 2021-06-15T16:00:04Z

Great! I'm going to do a quick visual test when the test environment spins up.

github-actions · 2021-06-15T19:03:36Z

@eschutho Ephemeral environment spinning up at http://52.37.150.225:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

eschutho · 2021-06-15T19:57:15Z

@corbinrobb this looks great. I wasn't able to sort the columns in the test environment. Can you check and see if that is working for you? It should sort both ascending and descending.

corbinrobb · 2021-06-15T20:09:42Z

@eschutho Hmm, mine is working as expected. The button originally doesn't sort the columns and just displays the way the table is sent and the button toggles between alphabetical and the original. In the picture it looks like that specific table is in alphabetical order already? That is kinda weird though because I also didn't change the logic in the sort function I just shortened it using ternary operators. Does it work on any of the tables?

eschutho · 2021-06-15T20:29:38Z

You're right, @corbinrobb. That example didn't have anything to sort. I tested on a different list and it's working. I'll go ahead and merge. Thanks for the help on this!

corbinrobb · 2021-06-15T20:32:16Z

Whew! That's relieving, I got super worried for a moment there lol Thanks for everything! @eschutho

github-actions · 2021-06-15T21:13:12Z

Ephemeral environment shutdown and build artifacts deleted.

* Convert TableElement to typescript * Change type names to better match naming conventions in other files * Fix import order and update tests on TableElement * Remove defaultProps * Destructure the props * Use Rest and Spread syntax to condense props destructuring * Fix TypeScript errors and add comment to explain antd props and types weirdness * Remove comment, add consistency with other files, and use method chaining to make more concise Co-authored-by: Corbin Robb <corbin@Corbins-MacBook-Pro.local>

Convert TableElement to typescript

7ee1ade

pull-request-size Bot added the size/L label Jun 4, 2021

corbinrobb changed the title ~~[WIP] refactor: Convert TableElement to TypeScript~~ refactor: Convert TableElement to TypeScript Jun 4, 2021

Change type names to better match naming conventions in other files

0127b92

hughhhh marked this pull request as ready for review June 4, 2021 18:56

Fix import order and update tests on TableElement

1cad775

AAfghahi reviewed Jun 7, 2021

View reviewed changes

Comment thread superset-frontend/src/SqlLab/components/TableElement.tsx

Remove defaultProps

f422720

AAfghahi reviewed Jun 8, 2021

View reviewed changes

Comment thread superset-frontend/src/SqlLab/components/TableElement.tsx Outdated

Corbin Robb added 2 commits June 8, 2021 19:07

Destructure the props

e3191a9

Use Rest and Spread syntax to condense props destructuring

d3fc350

AAfghahi reviewed Jun 10, 2021

View reviewed changes

Comment thread superset-frontend/src/SqlLab/components/TableElement.tsx Outdated

Fix TypeScript errors and add comment to explain antd props and types…

4071c19

… weirdness

AAfghahi approved these changes Jun 10, 2021

View reviewed changes

eschutho reviewed Jun 14, 2021

View reviewed changes

Remove comment, add consistency with other files, and use method chai…

80ca81a

…ning to make more concise

eschutho approved these changes Jun 14, 2021

View reviewed changes

eschutho merged commit b179863 into apache:master Jun 15, 2021

corbinrobb deleted the corbinrobb/tableelement-to-typescript branch December 8, 2021 04:05

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 First shipped in 1.3.0 labels Mar 12, 2024

Conversation

corbinrobb commented Jun 4, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

hughhhh commented Jun 4, 2021

Uh oh!

github-actions Bot commented Jun 4, 2021

Uh oh!

github-actions Bot commented Jun 4, 2021

Uh oh!

codecov Bot commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

AAfghahi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AAfghahi Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eschutho left a comment

Choose a reason for hiding this comment

Uh oh!

eschutho commented Jun 14, 2021

Uh oh!

eschutho commented Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corbinrobb commented Jun 15, 2021

Uh oh!

eschutho commented Jun 15, 2021

Uh oh!

eschutho commented Jun 15, 2021

Uh oh!

github-actions Bot commented Jun 15, 2021

Uh oh!

eschutho commented Jun 15, 2021

Uh oh!

corbinrobb commented Jun 15, 2021

Uh oh!

eschutho commented Jun 15, 2021

Uh oh!

corbinrobb commented Jun 15, 2021

Uh oh!

github-actions Bot commented Jun 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Jun 4, 2021 •

edited

Loading

AAfghahi Jun 10, 2021 •

edited

Loading

eschutho commented Jun 15, 2021 •

edited

Loading