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 2021/05/26 00:03:54 UTC
[GitHub] [superset] corbinrobb opened a new pull request #14830: [WIP] refactor: Convert TableElement.jsx component from class to functional with hooks
corbinrobb opened a new pull request #14830:
URL: https://github.com/apache/superset/pull/14830
### SUMMARY
Update the TableElement class component to be a functional component using hooks.
Modified the tests slightly where needed to check the changes to DOM from updating the state instead of checking the component state itself. Was necessary because functional components using hooks don't have an accessible state like class components.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
The component should look and function the same
### TESTING INSTRUCTIONS
To run the tests for TableElement navigate to superset-frontend and run `npm run test -- spec/javascripts/sqllab/TableElement_spec.jsx`
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] 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.
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] hughhhh commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
hughhhh commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-852371928
@corbinrobb can you fix these issues to allow CI to pass
```
/home/runner/work/superset/superset/superset-frontend/src/SqlLab/components/TableElement.jsx
35:1 error `src/components/Icons` import should occur before import of `../../components/CopyToClipboard` import/order
70:9 error 'popSelectStar' is assigned a value but never used no-unused-vars
192:30 error Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>` arrow-body-style
264:36 error Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>` arrow-body-style
280:20 error Curly braces are unnecessary here react/jsx-curly-brace-presence
292:7 error Value must be omitted for boolean attributes react/jsx-boolean-value
```
--
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] [superset] eschutho commented on a change in pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #14830:
URL: https://github.com/apache/superset/pull/14830#discussion_r645113640
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -156,26 +137,28 @@ class TableElement extends React.PureComponent {
{keyLink}
<IconTooltip
className={
- `fa fa-sort-${!this.state.sortColumns ? 'alpha' : 'numeric'}-asc ` +
+ `fa fa-sort-${!sortColumns ? 'alpha' : 'numeric'}-asc ` +
Review comment:
small nit, and this is from earlier code, but for readability:
``fa fa-sort-${sortColumns ? 'numeric' : 'alpha' }-asc ` +`
--
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] [superset] corbinrobb commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-852420572
@hughhhh I got the linter errors figured out and I pushed up the latest commit. I agree with @AAfghahi about popSelectStar. I removed it from the component since it was throwing the unused variable linter error. It appears it wasn't being used before I started the PR either but not throwing the linter because it was a method on a class. Thanks lemme know if I should/need to do anything else with 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.
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] eschutho commented on a change in pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #14830:
URL: https://github.com/apache/superset/pull/14830#discussion_r645113640
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -156,26 +137,28 @@ class TableElement extends React.PureComponent {
{keyLink}
<IconTooltip
className={
- `fa fa-sort-${!this.state.sortColumns ? 'alpha' : 'numeric'}-asc ` +
+ `fa fa-sort-${!sortColumns ? 'alpha' : 'numeric'}-asc ` +
Review comment:
small nit, and this is from earlier code, but for readability:
`fa fa-sort-${sortColumns ? 'numeric' : 'alpha' }-asc ` +
--
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] [superset] corbinrobb commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-850661319
It was my pleasure! @yousoph I'm looking forward to helping 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] [superset] hughhhh commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
hughhhh commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-854164342
/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.
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] eschutho commented on a change in pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #14830:
URL: https://github.com/apache/superset/pull/14830#discussion_r645111959
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -126,12 +108,11 @@ class TableElement extends React.PureComponent {
);
}
return header;
- }
+ };
- renderControls() {
+ const renderControls = () => {
let keyLink;
- const { table } = this.props;
- if (table.indexes && table.indexes.length > 0) {
+ if (table?.indexes.length > 0) {
Review comment:
if (table?.indexes?.length)
0 will be falsy, and it won't be negative. Also looks like indexes could be null or undefined.
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -156,26 +137,28 @@ class TableElement extends React.PureComponent {
{keyLink}
<IconTooltip
className={
- `fa fa-sort-${!this.state.sortColumns ? 'alpha' : 'numeric'}-asc ` +
+ `fa fa-sort-${!sortColumns ? 'alpha' : 'numeric'}-asc ` +
Review comment:
small nit, and this is from earlier code, but for readability:
``fa fa-sort-${sortColumns ? 'numeric' : 'alpha' }-asc ` +`
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -156,26 +137,28 @@ class TableElement extends React.PureComponent {
{keyLink}
<IconTooltip
className={
- `fa fa-sort-${!this.state.sortColumns ? 'alpha' : 'numeric'}-asc ` +
+ `fa fa-sort-${!sortColumns ? 'alpha' : 'numeric'}-asc ` +
Review comment:
small nit, and this is from earlier code, but for readability:
`fa fa-sort-${sortColumns ? 'numeric' : 'alpha' }-asc ` +
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -253,33 +232,54 @@ class TableElement extends React.PureComponent {
const metadata = (
<div
- onMouseEnter={() => this.setHover(true)}
- onMouseLeave={() => this.setHover(false)}
+ onMouseEnter={() => setHover(true)}
+ onMouseLeave={() => setHover(false)}
css={{ paddingTop: 6 }}
>
- {this.renderWell()}
+ {renderWell()}
<div>
- {cols &&
- cols.map(col => <ColumnElement column={col} key={col.name} />)}
+ {cols?.map(col => (
+ <ColumnElement column={col} key={col.name} />
+ ))}
</div>
</div>
);
return metadata;
- }
+ };
- render() {
- return (
- <Collapse.Panel
- {...this.props}
- header={this.renderHeader()}
- className="TableElement"
- forceRender="true"
- >
- {this.renderBody()}
- </Collapse.Panel>
- );
- }
-}
+ const collapseExpandIcon = () => (
+ <IconTooltip
+ style={{
+ position: 'fixed',
+ right: '16px',
+ left: 'auto',
+ fontSize: '12px',
+ transform: 'rotate(90deg)',
+ display: 'flex',
+ alignItems: 'center',
+ }}
+ aria-label="Collapse"
+ tooltip={t(`${!isActive ? 'Expand' : 'Collapse'} table preview`)}
Review comment:
can you do the same thing here?
tooltip={t(`${isActive ? 'Collapse' : 'Expand'} table preview`)}
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -126,12 +108,11 @@ class TableElement extends React.PureComponent {
);
}
return header;
- }
+ };
- renderControls() {
+ const renderControls = () => {
let keyLink;
- const { table } = this.props;
- if (table.indexes && table.indexes.length > 0) {
+ if (table?.indexes.length > 0) {
Review comment:
a lot of people still prefer foo.length > 0, so it's not wrong, just a personal preference. :)
--
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] [superset] lyndsiWilliams commented on a change in pull request #14830: [WIP] refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on a change in pull request #14830:
URL: https://github.com/apache/superset/pull/14830#discussion_r639337528
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -126,11 +119,10 @@ class TableElement extends React.PureComponent {
);
}
return header;
- }
+ };
- renderControls() {
+ const renderControls = () => {
let keyLink;
- const { table } = this.props;
if (table.indexes && table.indexes.length > 0) {
Review comment:
This can also be simplified to just `(table?.indexes)`
--
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] [superset] corbinrobb commented on a change in pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on a change in pull request #14830:
URL: https://github.com/apache/superset/pull/14830#discussion_r640875347
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -32,6 +32,7 @@ import ColumnElement from './ColumnElement';
import ShowSQL from './ShowSQL';
import ModalTrigger from '../../components/ModalTrigger';
import Loading from '../../components/Loading';
+import { RightOutlined } from '@ant-design/icons';
Review comment:
I see, thank you I will update that as soon as I can!
--
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] [superset] hughhhh commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
hughhhh commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-854164342
/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.
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] edited a comment on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-852544975
# [Codecov](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#14830](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (967fe57) into [master](https://codecov.io/gh/apache/superset/commit/6d33432b5846b24e8d71d923f8eb87fb6e51fa3d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6d33432) will **decrease** coverage by `0.01%`.
> The diff coverage is `88.23%`.
[](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #14830 +/- ##
==========================================
- Coverage 77.62% 77.61% -0.02%
==========================================
Files 962 965 +3
Lines 49078 49471 +393
Branches 6155 6259 +104
==========================================
+ Hits 38099 38399 +300
- Misses 10775 10872 +97
+ Partials 204 200 -4
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `72.48% <88.23%> (+0.06%)` | :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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-frontend/src/SqlLab/components/TableElement.jsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmxlRWxlbWVudC5qc3g=) | `92.00% <88.23%> (+2.97%)` | :arrow_up: |
| [...tersConfigModal/FiltersConfigForm/DefaultValue.tsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL0RlZmF1bHRWYWx1ZS50c3g=) | `26.31% <0.00%> (-62.58%)` | :arrow_down: |
| [...tend/src/filters/components/Time/transformProps.ts](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9UaW1lL3RyYW5zZm9ybVByb3BzLnRz) | `66.66% <0.00%> (-22.23%)` | :arrow_down: |
| [...ilters/FilterBar/FilterControls/FilterControls.tsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0ZpbHRlckNvbnRyb2xzL0ZpbHRlckNvbnRyb2xzLnRzeA==) | `77.58% <0.00%> (-17.16%)` | :arrow_down: |
| [...rset-frontend/src/components/IconTooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvblRvb2x0aXAvaW5kZXgudHN4) | `88.88% <0.00%> (-11.12%)` | :arrow_down: |
| [...nts/controls/DateFilterControl/DateFilterLabel.tsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRlRmlsdGVyQ29udHJvbC9EYXRlRmlsdGVyTGFiZWwudHN4) | `73.72% <0.00%> (-9.33%)` | :arrow_down: |
| [superset-frontend/src/utils/urlUtils.ts](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3V0aWxzL3VybFV0aWxzLnRz) | `53.33% <0.00%> (-8.21%)` | :arrow_down: |
| [superset-frontend/src/dataMask/actions.ts](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2RhdGFNYXNrL2FjdGlvbnMudHM=) | `71.42% <0.00%> (-7.15%)` | :arrow_down: |
| [...tend/src/filters/components/Select/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvY29udHJvbFBhbmVsLnRz) | `58.33% <0.00%> (-5.31%)` | :arrow_down: |
| [...nd/src/explore/components/DataTablesPane/index.tsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9EYXRhVGFibGVzUGFuZS9pbmRleC50c3g=) | `83.90% <0.00%> (-4.99%)` | :arrow_down: |
| ... and [73 more](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6d33432...967fe57](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] [superset] eschutho commented on a change in pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #14830:
URL: https://github.com/apache/superset/pull/14830#discussion_r645118781
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -126,12 +108,11 @@ class TableElement extends React.PureComponent {
);
}
return header;
- }
+ };
- renderControls() {
+ const renderControls = () => {
let keyLink;
- const { table } = this.props;
- if (table.indexes && table.indexes.length > 0) {
+ if (table?.indexes.length > 0) {
Review comment:
a lot of people still prefer foo.length > 0, so it's not wrong, just a personal preference. :)
--
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] [superset] AAfghahi commented on a change in pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #14830:
URL: https://github.com/apache/superset/pull/14830#discussion_r640867024
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -32,6 +32,7 @@ import ColumnElement from './ColumnElement';
import ShowSQL from './ShowSQL';
import ModalTrigger from '../../components/ModalTrigger';
import Loading from '../../components/Loading';
+import { RightOutlined } from '@ant-design/icons';
Review comment:
Hey, we actually use this for our Icons:
https://github.com/apache/superset/blob/master/superset-frontend/src/components/Icon/index.tsx
THis is a place where we use it which might help:
https://github.com/apache/superset/blob/master/superset-frontend/src/views/CRUD/data/query/QueryList.tsx
--
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] [superset] AAfghahi commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-852408801
> @corbinrobb can you fix these issues to allow CI to pass
>
> ```
> /home/runner/work/superset/superset/superset-frontend/src/SqlLab/components/TableElement.jsx
> 35:1 error `src/components/Icons` import should occur before import of `../../components/CopyToClipboard` import/order
> 70:9 error 'popSelectStar' is assigned a value but never used no-unused-vars
> 192:30 error Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>` arrow-body-style
> 264:36 error Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>` arrow-body-style
> 280:20 error Curly braces are unnecessary here react/jsx-curly-brace-presence
> 292:7 error Value must be omitted for boolean attributes react/jsx-boolean-value
> ```
So I actually think popSelectStar is depreciated. Might be worth setting up a test env to check to make sure.
--
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] [superset] corbinrobb commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-849812342
***
Figured it would make sense to grab a ticket that is also making changes in this component.
Please let me know if this should be in another PR or if there's a better way to handle it. Thanks!
***
## [sql lab]'copy' and 'collapse/expand' icons do not have tooltip in the table review panel
### SUMMARY
Fixed an issue where the tooltip for the copy/paste was not showing up and added a tooltip to the collapse/expand icon on the table element.
Updated tests where necessary.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
### TESTING INSTRUCTIONS
To run the tests for TableElement navigate to superset-frontend and run `npm run test -- spec/javascripts/sqllab/TableElement_spec.jsx`
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] 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.
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 #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-854165503
@hughhhh Ephemeral environment spinning up at http://34.210.58.81: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.
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 #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-854241715
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.
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] eschutho commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-850009668
@yousoph how do these tooltips 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] [superset] eschutho commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-854172804
This is great @corbinrobb, thanks so much! I just left a few nits, and some of them are from earlier code that I thought I'd see if we could sneak 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] [superset] corbinrobb commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-854203514
No problem at all! @eschutho I like all the changes! Much more concise and readable. I will be pushing them up in just a moment.
--
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] [superset] corbinrobb commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-854203514
No problem at all! @eschutho I like all the changes! Much more concise and readable. I will be pushing them up in just a moment.
--
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] [superset] betodealmeida merged pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
betodealmeida merged pull request #14830:
URL: https://github.com/apache/superset/pull/14830
--
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] [superset] codecov[bot] edited a comment on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-852544975
--
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] [superset] eschutho commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-854172804
This is great @corbinrobb, thanks so much! I just left a few nits, and some of them are from earlier code that I thought I'd see if we could sneak 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] [superset] github-actions[bot] commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-854165503
--
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] [superset] lyndsiWilliams commented on a change in pull request #14830: [WIP] refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on a change in pull request #14830:
URL: https://github.com/apache/superset/pull/14830#discussion_r639331793
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -253,33 +243,31 @@ class TableElement extends React.PureComponent {
const metadata = (
<div
- onMouseEnter={() => this.setHover(true)}
- onMouseLeave={() => this.setHover(false)}
+ onMouseEnter={() => setHover(true)}
+ onMouseLeave={() => setHover(false)}
css={{ paddingTop: 6 }}
>
- {this.renderWell()}
+ {renderWell()}
<div>
{cols &&
cols.map(col => <ColumnElement column={col} key={col.name} />)}
Review comment:
This can be simplified to `{cols?.map(col => <ColumnElement column={col} key={col.name} />)}`
--
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] [superset] AAfghahi commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-849850174
Heya, for the testing instructions, I think that adding how to visually test for it might be best. So something short and along the lines of ---> navigate to "x" then whatever would be good. So that someone who pulls your file, or if they create an ephemeral env could go and visually inspect 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.
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] corbinrobb commented on a change in pull request #14830: [WIP] refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on a change in pull request #14830:
URL: https://github.com/apache/superset/pull/14830#discussion_r639336881
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -253,33 +243,31 @@ class TableElement extends React.PureComponent {
const metadata = (
<div
- onMouseEnter={() => this.setHover(true)}
- onMouseLeave={() => this.setHover(false)}
+ onMouseEnter={() => setHover(true)}
+ onMouseLeave={() => setHover(false)}
css={{ paddingTop: 6 }}
>
- {this.renderWell()}
+ {renderWell()}
<div>
{cols &&
cols.map(col => <ColumnElement column={col} key={col.name} />)}
Review comment:
Sending up a commit!
--
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] [superset] eschutho commented on a change in pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #14830:
URL: https://github.com/apache/superset/pull/14830#discussion_r645111959
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -126,12 +108,11 @@ class TableElement extends React.PureComponent {
);
}
return header;
- }
+ };
- renderControls() {
+ const renderControls = () => {
let keyLink;
- const { table } = this.props;
- if (table.indexes && table.indexes.length > 0) {
+ if (table?.indexes.length > 0) {
Review comment:
if (table?.indexes?.length)
0 will be falsy, and it won't be negative. Also looks like indexes could be null or undefined.
--
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] [superset] codecov[bot] edited a comment on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-852544975
# [Codecov](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#14830](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c8efead) into [master](https://codecov.io/gh/apache/superset/commit/6d33432b5846b24e8d71d923f8eb87fb6e51fa3d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6d33432) will **decrease** coverage by `0.02%`.
> The diff coverage is `88.23%`.
> :exclamation: Current head c8efead differs from pull request most recent head 967fe57. Consider uploading reports for the commit 967fe57 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #14830 +/- ##
==========================================
- Coverage 77.62% 77.60% -0.03%
==========================================
Files 962 964 +2
Lines 49078 49322 +244
Branches 6155 6235 +80
==========================================
+ Hits 38099 38278 +179
- Misses 10775 10844 +69
+ Partials 204 200 -4
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `72.42% <88.23%> (+<0.01%)` | :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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-frontend/src/SqlLab/components/TableElement.jsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmxlRWxlbWVudC5qc3g=) | `92.00% <88.23%> (+2.97%)` | :arrow_up: |
| [...tersConfigModal/FiltersConfigForm/DefaultValue.tsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL0RlZmF1bHRWYWx1ZS50c3g=) | `26.31% <0.00%> (-62.58%)` | :arrow_down: |
| [...tend/src/filters/components/Time/transformProps.ts](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9UaW1lL3RyYW5zZm9ybVByb3BzLnRz) | `66.66% <0.00%> (-22.23%)` | :arrow_down: |
| [...rset-frontend/src/components/IconTooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvblRvb2x0aXAvaW5kZXgudHN4) | `88.88% <0.00%> (-11.12%)` | :arrow_down: |
| [...d/src/dashboard/components/gridComponents/Tabs.jsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL1RhYnMuanN4) | `76.31% <0.00%> (-11.06%)` | :arrow_down: |
| [...nts/controls/DateFilterControl/DateFilterLabel.tsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRlRmlsdGVyQ29udHJvbC9EYXRlRmlsdGVyTGFiZWwudHN4) | `73.72% <0.00%> (-9.33%)` | :arrow_down: |
| [superset-frontend/src/utils/urlUtils.ts](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3V0aWxzL3VybFV0aWxzLnRz) | `53.33% <0.00%> (-8.21%)` | :arrow_down: |
| [...tend/src/filters/components/Select/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvY29udHJvbFBhbmVsLnRz) | `58.33% <0.00%> (-5.31%)` | :arrow_down: |
| [...nd/src/explore/components/DataTablesPane/index.tsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9EYXRhVGFibGVzUGFuZS9pbmRleC50c3g=) | `83.90% <0.00%> (-4.99%)` | :arrow_down: |
| [...ontrols/FilterControl/AdhocFilterControl/index.jsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJDb250cm9sL0FkaG9jRmlsdGVyQ29udHJvbC9pbmRleC5qc3g=) | `54.61% <0.00%> (-3.08%)` | :arrow_down: |
| ... and [57 more](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6d33432...967fe57](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] [superset] corbinrobb commented on a change in pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
corbinrobb commented on a change in pull request #14830:
URL: https://github.com/apache/superset/pull/14830#discussion_r639653202
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -126,11 +119,10 @@ class TableElement extends React.PureComponent {
);
}
return header;
- }
+ };
- renderControls() {
+ const renderControls = () => {
let keyLink;
- const { table } = this.props;
if (table.indexes && table.indexes.length > 0) {
Review comment:
Pushed up changes!
--
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] [superset] yousoph commented on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
yousoph commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-850621647
Nice! Looking good to me, thanks @corbinrobb!
--
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] [superset] betodealmeida merged pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
betodealmeida merged pull request #14830:
URL: https://github.com/apache/superset/pull/14830
--
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] [superset] corbinrobb edited a comment on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
corbinrobb edited a comment on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-849812342
***
Figured it would make sense to grab a ticket that is also making changes in this component.
Please let me know if this should be in another PR or if there's a better way to handle it. Thanks!
***
## [sql lab]'copy' and 'collapse/expand' icons do not have tooltip in the table review panel
### SUMMARY
Fixed an issue where the tooltip for the copy/paste was not showing up and added a tooltip to the collapse/expand icon on the table element.
Updated tests where necessary.
### BEFORE
### AFTER
### 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 table and below the drop-down some TableElement components should appear
- Hover your mouse towards the top right corner of the component to use the toolbox and collapse/expand icons
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] 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.
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 #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-852544975
# [Codecov](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#14830](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c8efead) into [master](https://codecov.io/gh/apache/superset/commit/6d33432b5846b24e8d71d923f8eb87fb6e51fa3d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6d33432) will **decrease** coverage by `0.02%`.
> The diff coverage is `88.23%`.
[](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #14830 +/- ##
==========================================
- Coverage 77.62% 77.60% -0.03%
==========================================
Files 962 964 +2
Lines 49078 49322 +244
Branches 6155 6235 +80
==========================================
+ Hits 38099 38278 +179
- Misses 10775 10844 +69
+ Partials 204 200 -4
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `72.42% <88.23%> (+<0.01%)` | :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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-frontend/src/SqlLab/components/TableElement.jsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmxlRWxlbWVudC5qc3g=) | `92.00% <88.23%> (+2.97%)` | :arrow_up: |
| [...tersConfigModal/FiltersConfigForm/DefaultValue.tsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL0RlZmF1bHRWYWx1ZS50c3g=) | `26.31% <0.00%> (-62.58%)` | :arrow_down: |
| [...tend/src/filters/components/Time/transformProps.ts](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9UaW1lL3RyYW5zZm9ybVByb3BzLnRz) | `66.66% <0.00%> (-22.23%)` | :arrow_down: |
| [...rset-frontend/src/components/IconTooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvblRvb2x0aXAvaW5kZXgudHN4) | `88.88% <0.00%> (-11.12%)` | :arrow_down: |
| [...d/src/dashboard/components/gridComponents/Tabs.jsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL1RhYnMuanN4) | `76.31% <0.00%> (-11.06%)` | :arrow_down: |
| [...nts/controls/DateFilterControl/DateFilterLabel.tsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRlRmlsdGVyQ29udHJvbC9EYXRlRmlsdGVyTGFiZWwudHN4) | `73.72% <0.00%> (-9.33%)` | :arrow_down: |
| [superset-frontend/src/utils/urlUtils.ts](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3V0aWxzL3VybFV0aWxzLnRz) | `53.33% <0.00%> (-8.21%)` | :arrow_down: |
| [...tend/src/filters/components/Select/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvY29udHJvbFBhbmVsLnRz) | `58.33% <0.00%> (-5.31%)` | :arrow_down: |
| [...nd/src/explore/components/DataTablesPane/index.tsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9EYXRhVGFibGVzUGFuZS9pbmRleC50c3g=) | `83.90% <0.00%> (-4.99%)` | :arrow_down: |
| [...ontrols/FilterControl/AdhocFilterControl/index.jsx](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJDb250cm9sL0FkaG9jRmlsdGVyQ29udHJvbC9pbmRleC5qc3g=) | `54.61% <0.00%> (-3.08%)` | :arrow_down: |
| ... and [57 more](https://codecov.io/gh/apache/superset/pull/14830/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [6d33432...c8efead](https://codecov.io/gh/apache/superset/pull/14830?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] [superset] eschutho commented on a change in pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #14830:
URL: https://github.com/apache/superset/pull/14830#discussion_r645118132
##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -253,33 +232,54 @@ class TableElement extends React.PureComponent {
const metadata = (
<div
- onMouseEnter={() => this.setHover(true)}
- onMouseLeave={() => this.setHover(false)}
+ onMouseEnter={() => setHover(true)}
+ onMouseLeave={() => setHover(false)}
css={{ paddingTop: 6 }}
>
- {this.renderWell()}
+ {renderWell()}
<div>
- {cols &&
- cols.map(col => <ColumnElement column={col} key={col.name} />)}
+ {cols?.map(col => (
+ <ColumnElement column={col} key={col.name} />
+ ))}
</div>
</div>
);
return metadata;
- }
+ };
- render() {
- return (
- <Collapse.Panel
- {...this.props}
- header={this.renderHeader()}
- className="TableElement"
- forceRender="true"
- >
- {this.renderBody()}
- </Collapse.Panel>
- );
- }
-}
+ const collapseExpandIcon = () => (
+ <IconTooltip
+ style={{
+ position: 'fixed',
+ right: '16px',
+ left: 'auto',
+ fontSize: '12px',
+ transform: 'rotate(90deg)',
+ display: 'flex',
+ alignItems: 'center',
+ }}
+ aria-label="Collapse"
+ tooltip={t(`${!isActive ? 'Expand' : 'Collapse'} table preview`)}
Review comment:
can you do the same thing here?
tooltip={t(`${isActive ? 'Collapse' : 'Expand'} table preview`)}
--
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] [superset] corbinrobb edited a comment on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
corbinrobb edited a comment on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-849812342
***
Figured it would make sense to grab a ticket that is also making changes in this component.
Please let me know if this should be in another PR or if there's a better way to handle it. Thanks!
***
## [sql lab]'copy' and 'collapse/expand' icons do not have tooltip in the table review panel
### SUMMARY
Fixed an issue where the tooltip for the copy/paste was not showing up and added a tooltip to the collapse/expand icon on the table element.
Updated tests where necessary.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
### TESTING INSTRUCTIONS
To run the tests for TableElement navigate to superset-frontend and run `npm run test -- spec/javascripts/sqllab/TableElement_spec.jsx`
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] 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.
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] corbinrobb edited a comment on pull request #14830: refactor: Convert TableElement.jsx component from class to functional with hooks
Posted by GitBox <gi...@apache.org>.
corbinrobb edited a comment on pull request #14830:
URL: https://github.com/apache/superset/pull/14830#issuecomment-849812342
***
Figured it would make sense to grab a ticket that is also making changes in this component.
Please let me know if this should be in another PR or if there's a better way to handle it. Thanks!
***
## [sql lab]'copy' and 'collapse/expand' icons do not have tooltip in the table review panel
### SUMMARY
Fixed an issue where the tooltip for the copy/paste was not showing up and added a tooltip to the collapse/expand icon on the table element.
Updated tests where necessary.
### BEFORE
### AFTER
### TESTING INSTRUCTIONS
To run the tests for TableElement navigate to superset-frontend and run `npm run test -- spec/javascripts/sqllab/TableElement_spec.jsx`
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] 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.
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