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/08 23:13:17 UTC
[GitHub] [superset] AAfghahi opened a new pull request #13016: Refactor: HighlightSql to Functional Component
AAfghahi opened a new pull request #13016:
URL: https://github.com/apache/superset/pull/13016
### SUMMARY
Changed HighlightSql.jsx from a class component to a functional component.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Showing that it is still working.
### 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 #13016: refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13016:
URL: https://github.com/apache/superset/pull/13016#discussion_r572454847
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,77 +41,66 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({ sql, rawSql, maxWidth, maxLines, shrink }) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ 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) : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ };
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ const generateModal = () => {
+ 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>
- ),
- });
- }
-
- render() {
- return (
- <ModalTrigger
- modalTitle={t('SQL')}
- triggerNode={this.triggerNode()}
- modalBody={this.state.modalBody}
- beforeOpen={this.generateModal.bind(this)}
- />
+ setModalBody(
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {internalRawSql}
+ </div>,
);
- }
+ };
+
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={() => generateModal()}
Review comment:
like this? `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 pull request #13016: refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #13016:
URL: https://github.com/apache/superset/pull/13016#issuecomment-775537363
> Those happened after I did a git pull on master, and I'm unsure how to get rid of them. I'll rebase tonight, and re-commit. Should I close this PR and create an entirely new one?
You should be able to clean up the git history with this PR.. Happy to help if it gets unruly.
----------------------------------------------------------------
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 #13016: Refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13016:
URL: https://github.com/apache/superset/pull/13016#discussion_r572450400
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,77 +41,66 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({ sql, rawSql, maxWidth, maxLines, shrink }) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ 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) : sql;
Review comment:
looks like you don't need to pass the arg here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #13016: refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
AAfghahi closed pull request #13016:
URL: https://github.com/apache/superset/pull/13016
----------------------------------------------------------------
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 #13016: refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13016:
URL: https://github.com/apache/superset/pull/13016#discussion_r572470705
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,77 +41,66 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({ sql, rawSql, maxWidth, maxLines, shrink }) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ 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) : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ };
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ const generateModal = () => {
+ 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>
- ),
- });
- }
-
- render() {
- return (
- <ModalTrigger
- modalTitle={t('SQL')}
- triggerNode={this.triggerNode()}
- modalBody={this.state.modalBody}
- beforeOpen={this.generateModal.bind(this)}
- />
+ setModalBody(
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {internalRawSql}
+ </div>,
);
- }
+ };
+
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={() => generateModal()}
Review comment:
Yeah that's what I had, it's because it sets state inside the method, I think.
----------------------------------------------------------------
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 #13016: refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13016:
URL: https://github.com/apache/superset/pull/13016#discussion_r572947603
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,77 +41,66 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({ sql, rawSql, maxWidth, maxLines, shrink }) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ 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) : sql;
Review comment:
I run into a bunch of syntax errors when I reformat this to be
```
const triggerNode = (
**code**
)
```
The biggest thing, I think, is that there is another const defined within it? Could that have something to do with it. I did some reading, and looked at a fair amount of previous code, and that was the only thing that this has that others don't.
SImilar when I replace () with a {}
----------------------------------------------------------------
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 #13016: refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13016:
URL: https://github.com/apache/superset/pull/13016#discussion_r572947603
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,77 +41,66 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({ sql, rawSql, maxWidth, maxLines, shrink }) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ 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) : sql;
Review comment:
I run into a bunch of syntax errors when I reformat this to be
```
const triggerNode = (
**code**
)
```
The biggest thing, I think, is that there is another const defined within it? Could that have something to do with it. I did some reading, and looked at a fair amount of previous code, and that was the only thing that this has that others don't.
SImilar when I replace () with a {}
----------------------------------------------------------------
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 #13016: Refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13016:
URL: https://github.com/apache/superset/pull/13016#discussion_r572449187
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,77 +41,66 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({ sql, rawSql, maxWidth, maxLines, shrink }) {
Review comment:
Might be cleaner to move the default properties to the argument list here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #13016: Refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on pull request #13016:
URL: https://github.com/apache/superset/pull/13016#issuecomment-775531301
Those happened after I did a git pull on master, and I'm unsure how to get rid of them. I'll rebase tonight, and re-commit. Should I close this PR and create an entirely new one?
----------------------------------------------------------------
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 #13016: refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13016:
URL: https://github.com/apache/superset/pull/13016#discussion_r572452409
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,77 +41,66 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({ sql, rawSql, maxWidth, maxLines, shrink }) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ 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) : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ };
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ const generateModal = () => {
+ 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>
- ),
- });
- }
-
- render() {
- return (
- <ModalTrigger
- modalTitle={t('SQL')}
- triggerNode={this.triggerNode()}
- modalBody={this.state.modalBody}
- beforeOpen={this.generateModal.bind(this)}
- />
+ setModalBody(
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {internalRawSql}
+ </div>,
);
- }
+ };
+
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={() => generateModal()}
Review comment:
you can just put the method here `generateModal` instead of wrapping it in the function.
----------------------------------------------------------------
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 #13016: refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13016:
URL: https://github.com/apache/superset/pull/13016#discussion_r572453453
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,77 +41,66 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({ sql, rawSql, maxWidth, maxLines, shrink }) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ 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) : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ };
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ const generateModal = () => {
+ 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>
- ),
- });
- }
-
- render() {
- return (
- <ModalTrigger
- modalTitle={t('SQL')}
- triggerNode={this.triggerNode()}
- modalBody={this.state.modalBody}
- beforeOpen={this.generateModal.bind(this)}
- />
+ setModalBody(
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {internalRawSql}
+ </div>,
);
- }
+ };
+
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ triggerNode={triggerNode()}
Review comment:
we don't need this method to rerun on every rerender. How about we add this value to state and update it only when shrink or sql change?
----------------------------------------------------------------
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 #13016: refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13016:
URL: https://github.com/apache/superset/pull/13016#discussion_r572453465
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,77 +41,66 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({ sql, rawSql, maxWidth, maxLines, shrink }) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ 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) : sql;
return (
<SyntaxHighlighter language="sql" style={github}>
{shownSql}
</SyntaxHighlighter>
);
- }
+ };
- generateModal() {
- let rawSql;
- if (this.props.rawSql && this.props.rawSql !== this.props.sql) {
- rawSql = (
+ const generateModal = () => {
+ 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>
- ),
- });
- }
-
- render() {
- return (
- <ModalTrigger
- modalTitle={t('SQL')}
- triggerNode={this.triggerNode()}
- modalBody={this.state.modalBody}
- beforeOpen={this.generateModal.bind(this)}
- />
+ setModalBody(
+ <div>
+ <h4>{t('Source SQL')}</h4>
+ <SyntaxHighlighter language="sql" style={github}>
+ {sql}
+ </SyntaxHighlighter>
+ {internalRawSql}
+ </div>,
);
- }
+ };
+
+ return (
+ <ModalTrigger
+ modalTitle={t('SQL')}
+ triggerNode={triggerNode()}
+ modalBody={modalBody}
+ beforeOpen={() => generateModal()}
Review comment:
that is how I originally had it, but it creates an infinite loop.
----------------------------------------------------------------
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 #13016: Refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13016:
URL: https://github.com/apache/superset/pull/13016#discussion_r572450834
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,77 +41,66 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({ sql, rawSql, maxWidth, maxLines, shrink }) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ 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) : sql;
Review comment:
(I see that was how it was earlier, so just a bit of proactive cleanup since we're here)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #13016: refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13016:
URL: https://github.com/apache/superset/pull/13016#discussion_r572451398
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,77 +41,66 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({ sql, rawSql, maxWidth, maxLines, shrink }) {
Review comment:
Ok, I will get to these tonight. 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 pull request #13016: Refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
eschutho commented on pull request #13016:
URL: https://github.com/apache/superset/pull/13016#issuecomment-775529956
@AAfghahi I think you're going to want to rebase from master, and or maybe run a `git submodule update`. It looks like there are some unwanted changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] AAfghahi commented on a change in pull request #13016: refactor: HighlightSql to Functional Component
Posted by GitBox <gi...@apache.org>.
AAfghahi commented on a change in pull request #13016:
URL: https://github.com/apache/superset/pull/13016#discussion_r572947603
##########
File path: superset-frontend/src/SqlLab/components/HighlightedSql.jsx
##########
@@ -41,77 +41,66 @@ const propTypes = {
shrink: PropTypes.bool,
};
-class HighlightedSql extends React.Component {
- constructor(props) {
- super(props);
- this.state = {
- modalBody: null,
- };
- }
+function HighlightedSql({ sql, rawSql, maxWidth, maxLines, shrink }) {
+ const [modalBody, setModalBody] = useState(null);
- shrinkSql() {
- const ssql = this.props.sql || '';
+ 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) : sql;
Review comment:
I run into a bunch of syntax errors when I reformat this to be
```
const triggerNode = (
**code**
)
```
The biggest thing, I think, is that there is another const defined within it? Could that have something to do with it. I did some reading, and looked at a fair amount of previous code, and that was the only thing that this has that others don't.
----------------------------------------------------------------
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