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/01 21:41:39 UTC
[GitHub] [superset] etr2460 commented on a change in pull request #12873: refactor: TemplateParamsEditor.jsx converted from class to functional component
etr2460 commented on a change in pull request #12873:
URL: https://github.com/apache/superset/pull/12873#discussion_r568157092
##########
File path: superset-frontend/src/SqlLab/components/TemplateParamsEditor.jsx
##########
@@ -68,15 +60,19 @@ export default class TemplateParamsEditor extends React.Component {
} catch (e) {
isValid = false;
}
- this.setState({ parsedJSON, isValid, codeText });
+ setState({ parsedJSON, isValid, codeText });
const newValue = isValid ? codeText : '{}';
- if (newValue !== this.props.code) {
- this.props.onChange(newValue);
+ if (newValue !== props.code) {
+ props.onChange(newValue);
}
- }
+ };
+
+ useEffect(() => {
+ onChange(codeText);
+ }, [codeText]);
- renderDoc() {
- return (
+ const renderDoc = () => {
Review comment:
this can be pulled out the of functional component since it doesn't rely on any state
##########
File path: superset-frontend/src/SqlLab/components/TemplateParamsEditor.jsx
##########
@@ -95,53 +91,56 @@ export default class TemplateParamsEditor extends React.Component {
syntax.
</p>
);
- }
+ return renderDocContent;
+ };
- renderModalBody() {
- return (
+ const renderModalBody = () => {
+ const renderModalBodyContent = (
<div>
- {this.renderDoc()}
+ {renderDoc()}
<StyledConfigEditor
keywords={[]}
- mode={this.props.language}
+ mode={props.language}
minLines={25}
maxLines={50}
- onChange={this.onChange}
+ onChange={onChange}
width="100%"
editorProps={{ $blockScrolling: true }}
enableLiveAutocompletion
- value={this.state.codeText}
+ value={setState(state.codeText)}
Review comment:
why's the value the result of the setState call here? That doesn't seem quite right
##########
File path: superset-frontend/src/SqlLab/components/SqlEditor.jsx
##########
@@ -62,7 +62,7 @@ import {
validateQuery,
} from '../actions/sqlLab';
-import TemplateParamsEditor from './TemplateParamsEditor';
+import TemplateParamsEditorRename from './TemplateParamsEditor';
Review comment:
Let's not rename this here
##########
File path: superset-frontend/src/SqlLab/components/TemplateParamsEditor.jsx
##########
@@ -42,23 +42,15 @@ const StyledConfigEditor = styled(ConfigEditor)`
}
`;
-export default class TemplateParamsEditor extends React.Component {
- constructor(props) {
- super(props);
- const codeText = props.code || '{}';
- this.state = {
- codeText,
- parsedJSON: null,
- isValid: true,
- };
- this.onChange = this.onChange.bind(this);
- }
-
- componentDidMount() {
- this.onChange(this.state.codeText);
- }
+export const TemplateParamsEditor = props => {
Review comment:
there's no need to export here since we're exporting below
##########
File path: superset-frontend/src/SqlLab/components/TemplateParamsEditor.jsx
##########
@@ -95,53 +91,56 @@ export default class TemplateParamsEditor extends React.Component {
syntax.
</p>
);
- }
+ return renderDocContent;
+ };
- renderModalBody() {
- return (
+ const renderModalBody = () => {
+ const renderModalBodyContent = (
<div>
- {this.renderDoc()}
+ {renderDoc()}
<StyledConfigEditor
keywords={[]}
- mode={this.props.language}
+ mode={props.language}
minLines={25}
maxLines={50}
- onChange={this.onChange}
+ onChange={onChange}
width="100%"
editorProps={{ $blockScrolling: true }}
enableLiveAutocompletion
- value={this.state.codeText}
+ value={setState(state.codeText)}
/>
</div>
);
- }
+ return renderModalBodyContent;
+ };
- render() {
- const paramCount = this.state.parsedJSON
- ? Object.keys(this.state.parsedJSON).length
- : 0;
- return (
- <ModalTrigger
- modalTitle={t('Template parameters')}
- triggerNode={
- <div tooltip={t('Edit template parameters')} buttonSize="small">
- {`${t('Parameters')} `}
- <Badge count={paramCount} />
- {!this.state.isValid && (
- <InfoTooltipWithTrigger
- icon="exclamation-triangle"
- bsStyle="danger"
- tooltip={t('Invalid JSON')}
- label="invalid-json"
- />
- )}
- </div>
- }
- modalBody={this.renderModalBody(true)}
- />
- );
- }
-}
+ const paramCount = state.parsedJSON
+ ? Object.keys(state.parsedJSON).length
+ : 0;
+
+ return (
+ <ModalTrigger
+ modalTitle={t('Template parameters')}
+ triggerNode={
+ <div tooltip={t('Edit template parameters')} buttonSize="small">
+ {`${t('Parameters')} `}
+ <Badge count={paramCount} />
+ {!state.isValid && (
+ <InfoTooltipWithTrigger
+ icon="exclamation-triangle"
+ bsStyle="danger"
+ tooltip={t('Invalid JSON')}
+ label="invalid-json"
+ />
+ )}
+ </div>
+ }
+ modalBody={renderModalBody(true)}
Review comment:
what does the `true` argument here do?
##########
File path: superset-frontend/src/SqlLab/components/TemplateParamsEditor.jsx
##########
@@ -42,23 +42,15 @@ const StyledConfigEditor = styled(ConfigEditor)`
}
`;
-export default class TemplateParamsEditor extends React.Component {
- constructor(props) {
- super(props);
- const codeText = props.code || '{}';
- this.state = {
- codeText,
- parsedJSON: null,
- isValid: true,
- };
- this.onChange = this.onChange.bind(this);
- }
-
- componentDidMount() {
- this.onChange(this.state.codeText);
- }
+export const TemplateParamsEditor = props => {
Review comment:
also, it might be better to make this:
`function TemplateParamsEditor(props) {`
so that it's not an anonymous function in stack traces
----------------------------------------------------------------
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