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/02/09 17:26:43 UTC
[GitHub] [superset] AAfghahi opened a new pull request #13032: refactor: HighlightedSql into functional component
AAfghahi opened a new pull request #13032:
URL: https://github.com/apache/superset/pull/13032
### SUMMARY
Changed HighlightSql.jsx from a class component to a functional component.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
This is what highlight text looks like now.
### TEST PLAN
This passed all four tests in HighlightedSql_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
- [ ] 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] [superset] eschutho commented on a change in pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r575491667
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.tsx
##########
@@ -27,93 +26,92 @@ import ModalTrigger from '../../components/ModalTrigger';
SyntaxHighlighter.registerLanguage('sql', sql);
-const defaultProps = {
- maxWidth: 50,
- maxLines: 5,
- shrink: false,
-};
+interface propTypes {
+ sql: string;
+ rawSql?: string;
+ maxWidth?: number;
+ maxLines?: number;
+ shrink?: any;
+}
-const propTypes = {
- sql: PropTypes.string.isRequired,
- rawSql: PropTypes.string,
- maxWidth: PropTypes.number,
- maxLines: PropTypes.number,
- shrink: PropTypes.bool,
-};
+interface modalTypes {
+ rawSql?: string;
+ sql: string;
+}
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+interface nodeTypes {
Review comment:
This is great. The naming convention that we're using is TriggerNodeProps, HighlightedSqlModalProps, etc.
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #13032:
URL: https://github.com/apache/superset/pull/13032#issuecomment-777717814
Thank you @AAfghahi, this looks great! I manually tested and didn't find any regressions:
<img width="944" alt="_DEV__Superset" src="https://user-images.githubusercontent.com/5186919/107685289-aa98e200-6c58-11eb-8dbe-5cf03d37b47d.png">
----------------------------------------------------------------
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-io edited a comment on pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13032:
URL: https://github.com/apache/superset/pull/13032#issuecomment-776935946
# [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=h1) Report
> Merging [#13032](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=desc) (0fb3fb2) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `13.60%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13032 +/- ##
===========================================
+ Coverage 53.06% 66.66% +13.60%
===========================================
Files 489 491 +2
Lines 17314 28906 +11592
Branches 4482 0 -4482
===========================================
+ Hits 9187 19271 +10084
- Misses 8127 9635 +1508
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| python | `66.66% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `90.68% <ø> (ø)` | |
| [...nd/src/explore/components/ExploreViewContainer.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlVmlld0NvbnRhaW5lci5qc3g=) | | |
| [superset-frontend/src/setup/setupApp.ts](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQXBwLnRz) | | |
| [...-frontend/src/views/CRUD/welcome/ActivityTable.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvd2VsY29tZS9BY3Rpdml0eVRhYmxlLnRzeA==) | | |
| [...erset-frontend/src/SqlLab/components/SouthPane.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NvdXRoUGFuZS5qc3g=) | | |
| [...d/src/dashboard/components/gridComponents/Tabs.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL1RhYnMuanN4) | | |
| [...rset-frontend/src/common/components/CronPicker.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Nyb25QaWNrZXIudHN4) | | |
| [...nd/src/dashboard/util/getDetailedComponentWidth.js](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldERldGFpbGVkQ29tcG9uZW50V2lkdGguanM=) | | |
| [...ts/nativeFilters/FilterConfigModal/FilterScope.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQ29uZmlnTW9kYWwvRmlsdGVyU2NvcGUudHN4) | | |
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | | |
| ... and [968 more](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=footer). Last update [8b2df52...32b14e2](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573336259
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,86 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ const shrinkSql = useCallback(() => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ }, [maxLines, maxWidth, sql]);
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = useCallback(() => {
+ const shownSql = shrink ? shrinkSql(sql) : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ }, [shrink, shrinkSql, sql]);
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ const generateModal = useCallback(() => {
+ let internalRawSql;
+ if (rawSql && rawSql !== sql) {
+ internalRawSql = (
<div>
<h4>{t('Raw SQL')}</h4>
<SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
+ {rawSql}
</SyntaxHighlighter>
</div>
);
}
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
- {rawSql}
- </div>
- ),
- });
- }
+ setModalBody(
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {internalRawSql}
+ </div>,
+ );
+ }, [rawSql, sql]);
- render() {
- return (
+ const [modalTrigger, setModalTrigger] = useState(
Review comment:
can we put this up under line 45 just by convention? We can add a lint rule for this in the future.
----------------------------------------------------------------
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-io edited a comment on pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13032:
URL: https://github.com/apache/superset/pull/13032#issuecomment-776935946
# [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=h1) Report
> Merging [#13032](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=desc) (32b14e2) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `11.86%`.
> The diff coverage is `88.88%`.
[](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13032 +/- ##
===========================================
+ Coverage 53.06% 64.92% +11.86%
===========================================
Files 489 1038 +549
Lines 17314 49137 +31823
Branches 4482 5297 +815
===========================================
+ Hits 9187 31903 +22716
- Misses 8127 17021 +8894
- Partials 0 213 +213
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `61.85% <88.88%> (?)` | |
| python | `67.07% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...perset-frontend/src/views/CRUD/chart/ChartList.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvY2hhcnQvQ2hhcnRMaXN0LnRzeA==) | `71.65% <ø> (-2.66%)` | :arrow_down: |
| [...rontend/src/views/CRUD/dashboard/DashboardList.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGFzaGJvYXJkL0Rhc2hib2FyZExpc3QudHN4) | `73.87% <0.00%> (+7.56%)` | :arrow_up: |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `90.68% <ø> (ø)` | |
| [...-frontend/src/SqlLab/components/HighlightedSql.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0hpZ2hsaWdodGVkU3FsLnRzeA==) | `90.32% <94.11%> (ø)` | |
| [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/views/menu.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL21lbnUudHN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/views/index.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2luZGV4LnRzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/index.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9pbmRleC50c3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [955 more](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=footer). Last update [8b2df52...32b14e2](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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-io edited a comment on pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13032:
URL: https://github.com/apache/superset/pull/13032#issuecomment-776935946
# [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=h1) Report
> Merging [#13032](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=desc) (0fb3fb2) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `13.63%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13032 +/- ##
===========================================
+ Coverage 53.06% 66.69% +13.63%
===========================================
Files 489 491 +2
Lines 17314 28934 +11620
Branches 4482 0 -4482
===========================================
+ Hits 9187 19298 +10111
- Misses 8127 9636 +1509
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| python | `66.69% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `90.68% <ø> (ø)` | |
| [.../src/dashboard/util/serializeActiveFilterValues.js](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL3NlcmlhbGl6ZUFjdGl2ZUZpbHRlclZhbHVlcy5qcw==) | | |
| [...src/explore/components/controls/SpatialControl.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TcGF0aWFsQ29udHJvbC5qc3g=) | | |
| [...omponents/nativeFilters/FilterConfigModal/types.ts](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQ29uZmlnTW9kYWwvdHlwZXMudHM=) | | |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | | |
| [superset-frontend/src/explore/controls.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbHMuanN4) | | |
| [...UD/data/components/SyntaxHighlighterCopy/index.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9jb21wb25lbnRzL1N5bnRheEhpZ2hsaWdodGVyQ29weS9pbmRleC50c3g=) | | |
| [superset-frontend/src/components/FormRow.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRm9ybVJvdy5qc3g=) | | |
| [superset-frontend/src/setup/setupErrorMessages.ts](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRXJyb3JNZXNzYWdlcy50cw==) | | |
| [...end/src/components/Select/SupersetStyledSelect.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1N1cGVyc2V0U3R5bGVkU2VsZWN0LnRzeA==) | | |
| ... and [968 more](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=footer). Last update [8b2df52...32b14e2](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #13032:
URL: https://github.com/apache/superset/pull/13032#issuecomment-777706179
> > I'll do another manual qa when you're ready.
>
> Hello, I think I am ready. Would it be ok if I kept the ternary? Or is it better with the logical and?
Is this working? `{(rawSql && rawSql !== sql) && (` It would make more sense than returning null. If you don't have a second value, then a ternary is a little strange.
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573980921
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,80 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ modalBody={<Modal rawSql={rawSql} sql={sql} />}
+ triggerNode={
+ <TriggerNode
+ shrink={shrink}
+ sql={sql}
+ maxLines={maxLines}
+ maxWidth={maxWidth}
+ />
+ }
+ />
+ );
+}
+HighlightedSql.propTypes = propTypes;
+
+export default HighlightedSql;
- shrinkSql() {
- const ssql = this.props.sql || '';
+function TriggerNode({ shrink, sql, maxLines, maxWidth }) {
+ const shrinkSql = () => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ };
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = () => {
+ const shownSql = shrink ? shrinkSql() : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ };
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
- <div>
- <h4>{t('Raw SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
- </SyntaxHighlighter>
- </div>
- );
- }
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
+ return triggerNode;
Review comment:
This was why all my tests were failing. I was focusing on Modal body, since that is what I thought was producing the <pre>. But this makes a lot of sense. Thank you (edited to be more professional).
----------------------------------------------------------------
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-io edited a comment on pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13032:
URL: https://github.com/apache/superset/pull/13032#issuecomment-776935946
# [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=h1) Report
> Merging [#13032](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=desc) (a78bf5f) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `16.66%`.
> The diff coverage is `39.23%`.
[](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13032 +/- ##
===========================================
+ Coverage 53.06% 69.72% +16.66%
===========================================
Files 489 1038 +549
Lines 17314 49127 +31813
Branches 4482 5284 +802
===========================================
+ Hits 9187 34253 +25066
- Misses 8127 14750 +6623
- Partials 0 124 +124
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `53.42% <47.22%> (+0.36%)` | :arrow_up: |
| javascript | `61.89% <69.13%> (?)` | |
| python | `67.43% <25.12%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `64.28% <ø> (+11.50%)` | :arrow_up: |
| [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `54.76% <ø> (+4.76%)` | :arrow_up: |
| [...ontend/src/components/ListViewCard/ImageLoader.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXdDYXJkL0ltYWdlTG9hZGVyLnRzeA==) | `86.36% <0.00%> (+11.36%)` | :arrow_up: |
| [...set-frontend/src/components/URLShortLinkButton.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVVJMU2hvcnRMaW5rQnV0dG9uLmpzeA==) | `100.00% <ø> (ø)` | |
| [...src/dashboard/components/HeaderActionsDropdown.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0hlYWRlckFjdGlvbnNEcm9wZG93bi5qc3g=) | `71.23% <ø> (+3.66%)` | :arrow_up: |
| [...end/src/dashboard/components/StickyVerticalBar.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1N0aWNreVZlcnRpY2FsQmFyLnRzeA==) | `100.00% <ø> (ø)` | |
| [...rontend/src/explore/components/EmbedCodeButton.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FbWJlZENvZGVCdXR0b24uanN4) | `80.76% <ø> (ø)` | |
| [...ntend/src/explore/components/ExploreChartPanel.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlQ2hhcnRQYW5lbC5qc3g=) | `82.35% <ø> (-1.23%)` | :arrow_down: |
| [...d/src/filters/components/Range/AntdRangeFilter.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9BbnRkUmFuZ2VGaWx0ZXIudHN4) | `0.00% <0.00%> (ø)` | |
| [...set-frontend/src/filters/components/Range/index.ts](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9pbmRleC50cw==) | `30.00% <0.00%> (-45.00%)` | :arrow_down: |
| ... and [938 more](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=footer). Last update [8b2df52...7bd18b8](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573360078
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,86 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ const shrinkSql = useCallback(() => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ }, [maxLines, maxWidth, sql]);
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = useCallback(() => {
Review comment:
I don't think these useCallback method wrappers are going to work in this case because we're executing this method in the same file, so memoizing it doesn't help.
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,86 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ const shrinkSql = useCallback(() => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ }, [maxLines, maxWidth, sql]);
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = useCallback(() => {
+ const shownSql = shrink ? shrinkSql(sql) : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ }, [shrink, shrinkSql, sql]);
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ const generateModal = useCallback(() => {
+ let internalRawSql;
+ if (rawSql && rawSql !== sql) {
+ internalRawSql = (
<div>
<h4>{t('Raw SQL')}</h4>
<SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
+ {rawSql}
</SyntaxHighlighter>
</div>
);
}
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
- {rawSql}
- </div>
- ),
- });
- }
+ setModalBody(
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {internalRawSql}
+ </div>,
+ );
+ }, [rawSql, sql]);
- render() {
- return (
+ const [modalTrigger, setModalTrigger] = useState(
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={() => generateModal()}
+ />,
+ );
+
+ useEffect(() => {
+ setModalTrigger(
<ModalTrigger
modalTitle={t('SQL')}
- triggerNode={this.triggerNode()}
- modalBody={this.state.modalBody}
- beforeOpen={this.generateModal.bind(this)}
- />
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={generateModal}
+ />,
Review comment:
I know we had this in state before, but this is causing some duplication. I think we can simplify this component by a few things:
1) beforeOpen and modalBody are essentially the same thing. Let's remove beforeOpen altogether
2) instead of rerendering the triggerNode and modalBody on every render in this component, let's just just create a new functional component for each (you can keep them in this same file). That way we don't have to store anything in state or worry about what's rerendering when.
----------------------------------------------------------------
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 closed pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi closed pull request #13032:
URL: https://github.com/apache/superset/pull/13032
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573980921
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,80 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ modalBody={<Modal rawSql={rawSql} sql={sql} />}
+ triggerNode={
+ <TriggerNode
+ shrink={shrink}
+ sql={sql}
+ maxLines={maxLines}
+ maxWidth={maxWidth}
+ />
+ }
+ />
+ );
+}
+HighlightedSql.propTypes = propTypes;
+
+export default HighlightedSql;
- shrinkSql() {
- const ssql = this.props.sql || '';
+function TriggerNode({ shrink, sql, maxLines, maxWidth }) {
+ const shrinkSql = () => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ };
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = () => {
+ const shownSql = shrink ? shrinkSql() : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ };
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
- <div>
- <h4>{t('Raw SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
- </SyntaxHighlighter>
- </div>
- );
- }
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
+ return triggerNode;
Review comment:
wait, I think this is why my tests are failing. OMG it is. Thank you, thank you, thank you
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r575492015
##########
File path: superset/config.py
##########
@@ -623,13 +623,13 @@ class CeleryConfig: # pylint: disable=too-few-public-methods
"task": "email_reports.schedule_hourly",
"schedule": crontab(minute=1, hour="*"),
},
- 'reports.scheduler': {
- 'task': 'reports.scheduler',
- 'schedule': crontab(minute='*', hour='*'),
+ "reports.scheduler": {
+ "task": "reports.scheduler",
+ "schedule": crontab(minute="*", hour="*"),
},
- 'reports.prune_log': {
- 'task': 'reports.prune_log',
- 'schedule': crontab(minute=0, hour=0),
+ "reports.prune_log": {
+ "task": "reports.prune_log",
+ "schedule": crontab(minute=0, hour=0),
Review comment:
can you rebase? I think this is in master now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573378606
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,86 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ const shrinkSql = useCallback(() => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ }, [maxLines, maxWidth, sql]);
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = useCallback(() => {
+ const shownSql = shrink ? shrinkSql(sql) : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ }, [shrink, shrinkSql, sql]);
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ const generateModal = useCallback(() => {
+ let internalRawSql;
+ if (rawSql && rawSql !== sql) {
+ internalRawSql = (
<div>
<h4>{t('Raw SQL')}</h4>
<SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
+ {rawSql}
</SyntaxHighlighter>
</div>
);
}
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
- {rawSql}
- </div>
- ),
- });
- }
+ setModalBody(
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {internalRawSql}
+ </div>,
+ );
+ }, [rawSql, sql]);
- render() {
- return (
+ const [modalTrigger, setModalTrigger] = useState(
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={() => generateModal()}
+ />,
+ );
+
+ useEffect(() => {
+ setModalTrigger(
<ModalTrigger
modalTitle={t('SQL')}
- triggerNode={this.triggerNode()}
- modalBody={this.state.modalBody}
- beforeOpen={this.generateModal.bind(this)}
- />
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={generateModal}
+ />,
Review comment:
I really like this solution. Will do.
----------------------------------------------------------------
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-io edited a comment on pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13032:
URL: https://github.com/apache/superset/pull/13032#issuecomment-776935946
# [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=h1) Report
> Merging [#13032](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=desc) (32b14e2) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `11.84%`.
> The diff coverage is `88.88%`.
[](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13032 +/- ##
===========================================
+ Coverage 53.06% 64.90% +11.84%
===========================================
Files 489 1038 +549
Lines 17314 49109 +31795
Branches 4482 5297 +815
===========================================
+ Hits 9187 31876 +22689
- Misses 8127 17020 +8893
- Partials 0 213 +213
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `61.85% <88.88%> (?)` | |
| python | `67.04% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...perset-frontend/src/views/CRUD/chart/ChartList.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvY2hhcnQvQ2hhcnRMaXN0LnRzeA==) | `71.65% <ø> (-2.66%)` | :arrow_down: |
| [...rontend/src/views/CRUD/dashboard/DashboardList.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGFzaGJvYXJkL0Rhc2hib2FyZExpc3QudHN4) | `73.87% <0.00%> (+7.56%)` | :arrow_up: |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `90.68% <ø> (ø)` | |
| [...-frontend/src/SqlLab/components/HighlightedSql.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0hpZ2hsaWdodGVkU3FsLnRzeA==) | `90.32% <94.11%> (ø)` | |
| [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/views/menu.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL21lbnUudHN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/views/index.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2luZGV4LnRzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/index.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9pbmRleC50c3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [955 more](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=footer). Last update [8b2df52...32b14e2](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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-io edited a comment on pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13032:
URL: https://github.com/apache/superset/pull/13032#issuecomment-776935946
# [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=h1) Report
> Merging [#13032](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=desc) (6296a67) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `16.43%`.
> The diff coverage is `40.06%`.
[](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13032 +/- ##
===========================================
+ Coverage 53.06% 69.49% +16.43%
===========================================
Files 489 1038 +549
Lines 17314 49154 +31840
Branches 4482 5296 +814
===========================================
+ Hits 9187 34159 +24972
- Misses 8127 14870 +6743
- Partials 0 125 +125
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `53.37% <42.15%> (+0.31%)` | :arrow_up: |
| javascript | `61.84% <60.00%> (?)` | |
| python | `67.07% <25.12%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `64.28% <ø> (+11.50%)` | :arrow_up: |
| [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
| [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `77.02% <0.00%> (+0.31%)` | :arrow_up: |
| [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `54.76% <ø> (+4.76%)` | :arrow_up: |
| [...ontend/src/components/ListViewCard/ImageLoader.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXdDYXJkL0ltYWdlTG9hZGVyLnRzeA==) | `86.36% <0.00%> (+11.36%)` | :arrow_up: |
| [...set-frontend/src/components/URLShortLinkButton.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVVJMU2hvcnRMaW5rQnV0dG9uLmpzeA==) | `100.00% <ø> (ø)` | |
| [...src/dashboard/components/HeaderActionsDropdown.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0hlYWRlckFjdGlvbnNEcm9wZG93bi5qc3g=) | `71.23% <ø> (+3.66%)` | :arrow_up: |
| [...end/src/dashboard/components/StickyVerticalBar.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1N0aWNreVZlcnRpY2FsQmFyLnRzeA==) | `100.00% <ø> (ø)` | |
| [...perset-frontend/src/dashboard/containers/Chart.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0NoYXJ0LmpzeA==) | `100.00% <ø> (ø)` | |
| [...shboard/util/charts/getFormDataWithExtraFilters.ts](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2NoYXJ0cy9nZXRGb3JtRGF0YVdpdGhFeHRyYUZpbHRlcnMudHM=) | `93.10% <ø> (+1.10%)` | :arrow_up: |
| ... and [947 more](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=footer). Last update [8b2df52...32b14e2](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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 commented on a change in pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573089564
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,86 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ const shrinkSql = useCallback(() => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ }, [maxLines, maxWidth, sql]);
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = useCallback(() => {
+ const shownSql = shrink ? shrinkSql(sql) : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ }, [shrink, shrinkSql, sql]);
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ const generateModal = useCallback(() => {
+ let internalRawSql;
+ if (rawSql && rawSql !== sql) {
+ internalRawSql = (
<div>
<h4>{t('Raw SQL')}</h4>
<SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
+ {rawSql}
</SyntaxHighlighter>
</div>
);
}
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
- {rawSql}
- </div>
- ),
- });
- }
+ setModalBody(
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {internalRawSql}
+ </div>,
+ );
+ }, [rawSql, sql]);
- render() {
- return (
+ const [modalTrigger, setModalTrigger] = useState(
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={() => generateModal()}
+ />,
+ );
+
+ useEffect(() => {
+ setModalTrigger(
<ModalTrigger
modalTitle={t('SQL')}
- triggerNode={this.triggerNode()}
- modalBody={this.state.modalBody}
- beforeOpen={this.generateModal.bind(this)}
- />
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={() => generateModal()}
Review comment:
Nit, you should be able to use just `generateModal` here:
```js
beforeOpen={generateModal}
```
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on pull request #13032:
URL: https://github.com/apache/superset/pull/13032#issuecomment-776369862
Yes, I would be happy to do that. I was running into that with the other
component that I was working on as well.
On Tue, Feb 9, 2021 at 6:51 PM Elizabeth Thompson <no...@github.com>
wrote:
> *@eschutho* commented on this pull request.
> ------------------------------
>
> In superset-frontend/src/SqlLab/components/HighlightedSql.jsx
> <https://github.com/apache/superset/pull/13032#discussion_r573336259>:
>
> >
> - render() {
> - return (
> + const [modalTrigger, setModalTrigger] = useState(
>
> can we put this up under line 45 just by convention? We can add a lint
> rule for this in the future.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/superset/pull/13032#pullrequestreview-587125806>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ALVKTWE4F5B4C2M3VGKSYKDS6HDAJANCNFSM4XLMX6FA>
> .
>
--
Arash Afghahi
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r574115621
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,86 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ const shrinkSql = useCallback(() => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ }, [maxLines, maxWidth, sql]);
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = useCallback(() => {
Review comment:
Did some reading about this topic, and found: https://overreacted.io/a-complete-guide-to-useeffect/ to be pretty illuminating.
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r575497407
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.tsx
##########
@@ -27,93 +26,92 @@ import ModalTrigger from '../../components/ModalTrigger';
SyntaxHighlighter.registerLanguage('sql', sql);
-const defaultProps = {
- maxWidth: 50,
- maxLines: 5,
- shrink: false,
-};
+interface propTypes {
+ sql: string;
+ rawSql?: string;
+ maxWidth?: number;
+ maxLines?: number;
+ shrink?: any;
+}
-const propTypes = {
- sql: PropTypes.string.isRequired,
- rawSql: PropTypes.string,
- maxWidth: PropTypes.number,
- maxLines: PropTypes.number,
- shrink: PropTypes.bool,
-};
+interface modalTypes {
+ rawSql?: string;
+ sql: string;
+}
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+interface nodeTypes {
+ shrink: boolean;
+ sql: string;
+ maxLines: number;
+ maxWidth: number;
+}
- shrinkSql() {
- const ssql = this.props.sql || '';
+function TriggerNode({ shrink, sql, maxLines, maxWidth }: nodeTypes) {
+ const shrinkSql = () => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ };
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
- return (
+ return (
+ <SyntaxHighlighter language="sql" style={github}>
+ {shrink ? shrinkSql() : sql}
+ </SyntaxHighlighter>
+ );
+}
+
+function HighlightSqlModal({ rawSql, sql }: modalTypes) {
+ return (
+ <div>
+ <h4>{t('Source SQL')}</h4>
<SyntaxHighlighter language="sql" style={github}>
- {shownSql}
+ {sql}
</SyntaxHighlighter>
- );
- }
-
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ {rawSql && rawSql !== sql && (
<div>
<h4>{t('Raw SQL')}</h4>
<SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
+ {rawSql}
</SyntaxHighlighter>
</div>
- );
- }
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
- {rawSql}
- </div>
- ),
- });
- }
+ )}
+ </div>
+ );
+}
- render() {
- return (
- <ModalTrigger
- modalTitle={t('SQL')}
- triggerNode={this.triggerNode()}
- modalBody={this.state.modalBody}
- beforeOpen={this.generateModal.bind(this)}
- />
- );
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}: propTypes) {
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ modalBody={<HighlightSqlModal rawSql={rawSql} sql={sql} />}
+ triggerNode={
+ <TriggerNode
+ shrink={shrink}
+ sql={sql}
+ maxLines={maxLines}
+ maxWidth={maxWidth}
+ />
+ }
+ />
+ );
}
-HighlightedSql.propTypes = propTypes;
-HighlightedSql.defaultProps = defaultProps;
+// HighlightedSql.propTypes = propTypes;
Review comment:
Yeah, it seemed important so I didn't want to remove it right away. But this confirms that I should.
----------------------------------------------------------------
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 commented on a change in pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573089564
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,86 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ const shrinkSql = useCallback(() => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ }, [maxLines, maxWidth, sql]);
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = useCallback(() => {
+ const shownSql = shrink ? shrinkSql(sql) : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ }, [shrink, shrinkSql, sql]);
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ const generateModal = useCallback(() => {
+ let internalRawSql;
+ if (rawSql && rawSql !== sql) {
+ internalRawSql = (
<div>
<h4>{t('Raw SQL')}</h4>
<SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
+ {rawSql}
</SyntaxHighlighter>
</div>
);
}
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
- {rawSql}
- </div>
- ),
- });
- }
+ setModalBody(
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {internalRawSql}
+ </div>,
+ );
+ }, [rawSql, sql]);
- render() {
- return (
+ const [modalTrigger, setModalTrigger] = useState(
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={() => generateModal()}
+ />,
+ );
+
+ useEffect(() => {
+ setModalTrigger(
<ModalTrigger
modalTitle={t('SQL')}
- triggerNode={this.triggerNode()}
- modalBody={this.state.modalBody}
- beforeOpen={this.generateModal.bind(this)}
- />
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={() => generateModal()}
Review comment:
Nit, you should be able to use just `generateModal` here:
```js
beforeOpen={generateModal}
```
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r574166937
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,76 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ modalBody={<Modal rawSql={rawSql} sql={sql} />}
+ triggerNode={
+ <TriggerNode
+ shrink={shrink}
+ sql={sql}
+ maxLines={maxLines}
+ maxWidth={maxWidth}
+ />
+ }
+ />
+ );
+}
+HighlightedSql.propTypes = propTypes;
+
+export default HighlightedSql;
- shrinkSql() {
- const ssql = this.props.sql || '';
+function TriggerNode({ shrink, sql, maxLines, maxWidth }) {
+ const shrinkSql = () => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ };
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
- return (
- <SyntaxHighlighter language="sql" style={github}>
- {shownSql}
- </SyntaxHighlighter>
- );
- }
+ const shownSql = shrink ? shrinkSql() : sql;
+ return (
+ <SyntaxHighlighter language="sql" style={github}>
+ {shownSql}
Review comment:
looking great. One small nit: you prob don't need to declare shownSql on line 82 since you're not reusing it anywhere, and it's only one line. You can just put `{shrink ? shrinkSql() : sql}` here instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r574110927
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,80 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ modalBody={<Modal rawSql={rawSql} sql={sql} />}
+ triggerNode={
+ <TriggerNode
+ shrink={shrink}
+ sql={sql}
+ maxLines={maxLines}
+ maxWidth={maxWidth}
+ />
+ }
+ />
+ );
+}
+HighlightedSql.propTypes = propTypes;
+
+export default HighlightedSql;
- shrinkSql() {
- const ssql = this.props.sql || '';
+function TriggerNode({ shrink, sql, maxLines, maxWidth }) {
+ const shrinkSql = () => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ };
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = () => {
+ const shownSql = shrink ? shrinkSql() : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ };
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
- <div>
- <h4>{t('Raw SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
- </SyntaxHighlighter>
- </div>
- );
- }
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
+ return triggerNode();
Review comment:
changed and pushed, that does make it more elegant. Thank you for taking the time and reviewing these.
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r574169580
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,76 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ modalBody={<Modal rawSql={rawSql} sql={sql} />}
+ triggerNode={
+ <TriggerNode
+ shrink={shrink}
+ sql={sql}
+ maxLines={maxLines}
+ maxWidth={maxWidth}
+ />
+ }
+ />
+ );
+}
+HighlightedSql.propTypes = propTypes;
+
+export default HighlightedSql;
- shrinkSql() {
- const ssql = this.props.sql || '';
+function TriggerNode({ shrink, sql, maxLines, maxWidth }) {
+ const shrinkSql = () => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ };
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
- return (
- <SyntaxHighlighter language="sql" style={github}>
- {shownSql}
- </SyntaxHighlighter>
- );
- }
+ const shownSql = shrink ? shrinkSql() : sql;
+ return (
+ <SyntaxHighlighter language="sql" style={github}>
+ {shownSql}
+ </SyntaxHighlighter>
+ );
+}
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
- <div>
- <h4>{t('Raw SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
- </SyntaxHighlighter>
- </div>
- );
- }
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
+function Modal({ rawSql, sql }) {
+ const generateModal =
+ rawSql && rawSql !== sql ? (
+ <div>
+ <h4>{t('Raw SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
{rawSql}
- </div>
- ),
- });
- }
+ </SyntaxHighlighter>
+ </div>
+ ) : null;
- render() {
- return (
- <ModalTrigger
- modalTitle={t('SQL')}
- triggerNode={this.triggerNode()}
- modalBody={this.state.modalBody}
- beforeOpen={this.generateModal.bind(this)}
- />
- );
- }
+ return (
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {generateModal}
Review comment:
actually I just found one more.. similar to my last comment. You can just pull all of line 92-99 here.. no need to declare it above. You can also use the logical and (&&) operator instead of ternary if your "or" case is `null`.
----------------------------------------------------------------
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] nytai commented on a change in pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r575472478
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -27,12 +27,6 @@ import ModalTrigger from '../../components/ModalTrigger';
SyntaxHighlighter.registerLanguage('sql', sql);
-const defaultProps = {
- maxWidth: 50,
- maxLines: 5,
- shrink: false,
-};
-
const propTypes = {
Review comment:
Nit: can we convert this to `tsx` and use typescript to type these?
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on pull request #13032:
URL: https://github.com/apache/superset/pull/13032#issuecomment-777146651
> I'll do another manual qa when you're ready.
Hello, I think I am ready. Would it be ok if I kept the ternary? Or is it better with the logical and?
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573970520
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,86 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ const shrinkSql = useCallback(() => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ }, [maxLines, maxWidth, sql]);
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = useCallback(() => {
Review comment:
not sure about the linter. Let me know if you have a screenshot of 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] AAfghahi commented on a change in pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573379845
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,86 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ const shrinkSql = useCallback(() => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ }, [maxLines, maxWidth, sql]);
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = useCallback(() => {
Review comment:
I was wondering something about this, I used a lot of these useCallbacks because the linter was suggesting them. Reading more about them, I wasn't sure that it was necessary, and there were some articles about how they can slow down service. You mentioned earlier that editing the linter made sense, would this also be a case for that?
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573333239
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,86 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ const shrinkSql = useCallback(() => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ }, [maxLines, maxWidth, sql]);
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = useCallback(() => {
+ const shownSql = shrink ? shrinkSql(sql) : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ }, [shrink, shrinkSql, sql]);
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ const generateModal = useCallback(() => {
+ let internalRawSql;
+ if (rawSql && rawSql !== sql) {
+ internalRawSql = (
<div>
<h4>{t('Raw SQL')}</h4>
<SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
+ {rawSql}
</SyntaxHighlighter>
</div>
);
}
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
- {rawSql}
- </div>
- ),
- });
- }
+ setModalBody(
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {internalRawSql}
+ </div>,
+ );
+ }, [rawSql, sql]);
- render() {
- return (
+ const [modalTrigger, setModalTrigger] = useState(
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={() => generateModal()}
Review comment:
did we get `beforeOpen={generateModal}` to work?
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573961769
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,80 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ modalBody={<Modal rawSql={rawSql} sql={sql} />}
+ triggerNode={
+ <TriggerNode
+ shrink={shrink}
+ sql={sql}
+ maxLines={maxLines}
+ maxWidth={maxWidth}
+ />
+ }
+ />
+ );
+}
+HighlightedSql.propTypes = propTypes;
+
+export default HighlightedSql;
- shrinkSql() {
- const ssql = this.props.sql || '';
+function TriggerNode({ shrink, sql, maxLines, maxWidth }) {
+ const shrinkSql = () => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ };
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = () => {
+ const shownSql = shrink ? shrinkSql() : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ };
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
- <div>
- <h4>{t('Raw SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
- </SyntaxHighlighter>
- </div>
- );
- }
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
+ return triggerNode;
Review comment:
is this working? Looks like you're returning a function instead of html?
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573980921
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,80 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ modalBody={<Modal rawSql={rawSql} sql={sql} />}
+ triggerNode={
+ <TriggerNode
+ shrink={shrink}
+ sql={sql}
+ maxLines={maxLines}
+ maxWidth={maxWidth}
+ />
+ }
+ />
+ );
+}
+HighlightedSql.propTypes = propTypes;
+
+export default HighlightedSql;
- shrinkSql() {
- const ssql = this.props.sql || '';
+function TriggerNode({ shrink, sql, maxLines, maxWidth }) {
+ const shrinkSql = () => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ };
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = () => {
+ const shownSql = shrink ? shrinkSql() : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ };
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
- <div>
- <h4>{t('Raw SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
- </SyntaxHighlighter>
- </div>
- );
- }
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
+ return triggerNode;
Review comment:
This was why all my tests were failing. I was focusing on Modal body, since that is what I thought was producing the 'pre'. But this makes a lot of sense. Thank you (edited to be more professional).
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r575491883
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.tsx
##########
@@ -27,93 +26,92 @@ import ModalTrigger from '../../components/ModalTrigger';
SyntaxHighlighter.registerLanguage('sql', sql);
-const defaultProps = {
- maxWidth: 50,
- maxLines: 5,
- shrink: false,
-};
+interface propTypes {
+ sql: string;
+ rawSql?: string;
+ maxWidth?: number;
+ maxLines?: number;
+ shrink?: any;
+}
-const propTypes = {
- sql: PropTypes.string.isRequired,
- rawSql: PropTypes.string,
- maxWidth: PropTypes.number,
- maxLines: PropTypes.number,
- shrink: PropTypes.bool,
-};
+interface modalTypes {
+ rawSql?: string;
+ sql: string;
+}
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+interface nodeTypes {
+ shrink: boolean;
+ sql: string;
+ maxLines: number;
+ maxWidth: number;
+}
- shrinkSql() {
- const ssql = this.props.sql || '';
+function TriggerNode({ shrink, sql, maxLines, maxWidth }: nodeTypes) {
+ const shrinkSql = () => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ };
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
- return (
+ return (
+ <SyntaxHighlighter language="sql" style={github}>
+ {shrink ? shrinkSql() : sql}
+ </SyntaxHighlighter>
+ );
+}
+
+function HighlightSqlModal({ rawSql, sql }: modalTypes) {
+ return (
+ <div>
+ <h4>{t('Source SQL')}</h4>
<SyntaxHighlighter language="sql" style={github}>
- {shownSql}
+ {sql}
</SyntaxHighlighter>
- );
- }
-
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ {rawSql && rawSql !== sql && (
<div>
<h4>{t('Raw SQL')}</h4>
<SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
+ {rawSql}
</SyntaxHighlighter>
</div>
- );
- }
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
- {rawSql}
- </div>
- ),
- });
- }
+ )}
+ </div>
+ );
+}
- render() {
- return (
- <ModalTrigger
- modalTitle={t('SQL')}
- triggerNode={this.triggerNode()}
- modalBody={this.state.modalBody}
- beforeOpen={this.generateModal.bind(this)}
- />
- );
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}: propTypes) {
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ modalBody={<HighlightSqlModal rawSql={rawSql} sql={sql} />}
+ triggerNode={
+ <TriggerNode
+ shrink={shrink}
+ sql={sql}
+ maxLines={maxLines}
+ maxWidth={maxWidth}
+ />
+ }
+ />
+ );
}
-HighlightedSql.propTypes = propTypes;
-HighlightedSql.defaultProps = defaultProps;
+// HighlightedSql.propTypes = propTypes;
Review comment:
remove?
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r574103498
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,80 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ modalBody={<Modal rawSql={rawSql} sql={sql} />}
+ triggerNode={
+ <TriggerNode
+ shrink={shrink}
+ sql={sql}
+ maxLines={maxLines}
+ maxWidth={maxWidth}
+ />
+ }
+ />
+ );
+}
+HighlightedSql.propTypes = propTypes;
+
+export default HighlightedSql;
- shrinkSql() {
- const ssql = this.props.sql || '';
+function TriggerNode({ shrink, sql, maxLines, maxWidth }) {
+ const shrinkSql = () => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ };
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = () => {
+ const shownSql = shrink ? shrinkSql() : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ };
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
- <div>
- <h4>{t('Raw SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
- </SyntaxHighlighter>
- </div>
- );
- }
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
+ return triggerNode();
Review comment:
might be easier to just return what you have on line 84. You can then remove triggerNode altogether.
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #13032:
URL: https://github.com/apache/superset/pull/13032#issuecomment-777718386
🏷 ready-to-merge
----------------------------------------------------------------
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-io commented on pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #13032:
URL: https://github.com/apache/superset/pull/13032#issuecomment-776935946
# [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=h1) Report
> Merging [#13032](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=desc) (fde9000) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `8.79%`.
> The diff coverage is `86.36%`.
[](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13032 +/- ##
==========================================
+ Coverage 53.06% 61.85% +8.79%
==========================================
Files 489 546 +57
Lines 17314 20152 +2838
Branches 4482 5271 +789
==========================================
+ Hits 9187 12465 +3278
+ Misses 8127 7474 -653
- Partials 0 213 +213
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `61.85% <86.36%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...perset-frontend/src/views/CRUD/chart/ChartList.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvY2hhcnQvQ2hhcnRMaXN0LnRzeA==) | `71.65% <ø> (-2.66%)` | :arrow_down: |
| [...rontend/src/views/CRUD/dashboard/DashboardList.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGFzaGJvYXJkL0Rhc2hib2FyZExpc3QudHN4) | `73.87% <0.00%> (+7.56%)` | :arrow_up: |
| [...-frontend/src/SqlLab/components/HighlightedSql.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0hpZ2hsaWdodGVkU3FsLmpzeA==) | `86.20% <90.47%> (+69.54%)` | :arrow_up: |
| [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/views/menu.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL21lbnUudHN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/views/index.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2luZGV4LnRzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/index.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9pbmRleC50c3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [456 more](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=footer). Last update [8b2df52...fde9000](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573333239
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,86 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ const shrinkSql = useCallback(() => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ }, [maxLines, maxWidth, sql]);
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = useCallback(() => {
+ const shownSql = shrink ? shrinkSql(sql) : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ }, [shrink, shrinkSql, sql]);
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ const generateModal = useCallback(() => {
+ let internalRawSql;
+ if (rawSql && rawSql !== sql) {
+ internalRawSql = (
<div>
<h4>{t('Raw SQL')}</h4>
<SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
+ {rawSql}
</SyntaxHighlighter>
</div>
);
}
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
- {rawSql}
- </div>
- ),
- });
- }
+ setModalBody(
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {internalRawSql}
+ </div>,
+ );
+ }, [rawSql, sql]);
- render() {
- return (
+ const [modalTrigger, setModalTrigger] = useState(
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={() => generateModal()}
Review comment:
did we get `beforeOpen={generateModal}` to work?
----------------------------------------------------------------
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 #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13032:
URL: https://github.com/apache/superset/pull/13032#discussion_r573364606
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,79 +35,86 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({
+ sql,
+ rawSql,
+ maxWidth = 50,
+ maxLines = 5,
+ shrink = false,
+}) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ const shrinkSql = useCallback(() => {
+ const ssql = sql || '';
let lines = ssql.split('\n');
- if (lines.length >= this.props.maxLines) {
- lines = lines.slice(0, this.props.maxLines);
+ if (lines.length >= maxLines) {
+ lines = lines.slice(0, maxLines);
lines.push('{...}');
}
return lines
.map(line => {
- if (line.length > this.props.maxWidth) {
- return `${line.slice(0, this.props.maxWidth)}{...}`;
+ if (line.length > maxWidth) {
+ return `${line.slice(0, maxWidth)}{...}`;
}
return line;
})
.join('\n');
- }
+ }, [maxLines, maxWidth, sql]);
- triggerNode() {
- const shownSql = this.props.shrink
- ? this.shrinkSql(this.props.sql)
- : this.props.sql;
+ const triggerNode = useCallback(() => {
+ const shownSql = shrink ? shrinkSql(sql) : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ }, [shrink, shrinkSql, sql]);
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ const generateModal = useCallback(() => {
+ let internalRawSql;
+ if (rawSql && rawSql !== sql) {
+ internalRawSql = (
<div>
<h4>{t('Raw SQL')}</h4>
<SyntaxHighlighter language="sql" style={github}>
- {this.props.rawSql}
+ {rawSql}
</SyntaxHighlighter>
</div>
);
}
- this.setState({
- modalBody: (
- <div>
- <h4>{t('Source SQL')}</h4>
- <SyntaxHighlighter language="sql" style={github}>
- {this.props.sql}
- </SyntaxHighlighter>
- {rawSql}
- </div>
- ),
- });
- }
+ setModalBody(
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {internalRawSql}
+ </div>,
+ );
+ }, [rawSql, sql]);
- render() {
- return (
+ const [modalTrigger, setModalTrigger] = useState(
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={() => generateModal()}
+ />,
+ );
+
+ useEffect(() => {
+ setModalTrigger(
<ModalTrigger
modalTitle={t('SQL')}
- triggerNode={this.triggerNode()}
- modalBody={this.state.modalBody}
- beforeOpen={this.generateModal.bind(this)}
- />
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={generateModal}
+ />,
Review comment:
you'll essentially be left with something like this:
```
function HighlightedSql({
sql,
rawSql,
maxWidth = 50,
maxLines = 5,
shrink = false,
}) {
return (
<ModalTrigger
modalTitle={t('SQL')}
triggerNode={
<TriggerNode
shrink={shrink}
sql={sql}
maxLines={maxLines}
maxWidth={maxWidth}
/>
}
modalBody={<Modal rawSql={rawSql} sql={sql} />}
/>
);
}
```
----------------------------------------------------------------
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-io edited a comment on pull request #13032: refactor: HighlightedSql into functional component
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13032:
URL: https://github.com/apache/superset/pull/13032#issuecomment-776935946
# [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=h1) Report
> Merging [#13032](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=desc) (32b14e2) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `11.62%`.
> The diff coverage is `88.88%`.
[](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13032 +/- ##
===========================================
+ Coverage 53.06% 64.68% +11.62%
===========================================
Files 489 1038 +549
Lines 17314 49109 +31795
Branches 4482 5297 +815
===========================================
+ Hits 9187 31767 +22580
- Misses 8127 17129 +9002
- Partials 0 213 +213
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `61.85% <88.88%> (?)` | |
| python | `66.66% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...perset-frontend/src/views/CRUD/chart/ChartList.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvY2hhcnQvQ2hhcnRMaXN0LnRzeA==) | `71.65% <ø> (-2.66%)` | :arrow_down: |
| [...rontend/src/views/CRUD/dashboard/DashboardList.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGFzaGJvYXJkL0Rhc2hib2FyZExpc3QudHN4) | `73.87% <0.00%> (+7.56%)` | :arrow_up: |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `90.68% <ø> (ø)` | |
| [...-frontend/src/SqlLab/components/HighlightedSql.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0hpZ2hsaWdodGVkU3FsLnRzeA==) | `90.32% <94.11%> (ø)` | |
| [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/views/menu.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL21lbnUudHN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/views/index.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2luZGV4LnRzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/index.tsx](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9pbmRleC50c3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [955 more](https://codecov.io/gh/apache/superset/pull/13032/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=footer). Last update [8b2df52...32b14e2](https://codecov.io/gh/apache/superset/pull/13032?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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