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 23:03:28 UTC
[GitHub] [superset] etr2460 commented on a change in pull request #12873: [WIP] 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_r568196505
##########
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);
- }
+function TemplateParamsEditor(props) {
Review comment:
optional:
you could make the argument here be
```javascript
function TemplateParamsEditor({ code, language }) {
```
so that you don't need to reference props anymore throughout the component
##########
File path: superset-frontend/src/SqlLab/components/TemplateParamsEditor.jsx
##########
@@ -68,80 +60,81 @@ 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);
}
- }
+ };
- renderDoc() {
- return (
- <p>
- {t('Assign a set of parameters as')}
- <code>JSON</code>
- {t('below (example:')}
- <code>{'{"my_table": "foo"}'}</code>
- {t('), and they become available in your SQL (example:')}
- <code>SELECT * FROM {'{{ my_table }}'} </code>
- {t(') by using')}
- <a
- href="https://superset.apache.org/sqllab.html#templating-with-jinja"
- target="_blank"
- rel="noopener noreferrer"
- >
- Jinja templating
- </a>{' '}
- syntax.
- </p>
- );
- }
+ useEffect(() => {
+ onChange(codeText);
+ }, [codeText]);
- renderModalBody() {
- return (
- <div>
- {this.renderDoc()}
- <StyledConfigEditor
- keywords={[]}
- mode={this.props.language}
- minLines={25}
- maxLines={50}
- onChange={this.onChange}
- width="100%"
- editorProps={{ $blockScrolling: true }}
- enableLiveAutocompletion
- value={this.state.codeText}
- />
- </div>
- );
- }
+ const renderDoc = (
+ <p>
+ {t('Assign a set of parameters as')}
+ <code>JSON</code>
+ {t('below (example:')}
+ <code>{'{"my_table": "foo"}'}</code>
+ {t('), and they become available in your SQL (example:')}
+ <code>SELECT * FROM {'{{ my_table }}'} </code>
+ {t(') by using')}
+ <a
+ href="https://superset.apache.org/sqllab.html#templating-with-jinja"
+ target="_blank"
+ rel="noopener noreferrer"
+ >
+ Jinja templating
+ </a>{' '}
+ syntax.
+ </p>
+ );
- 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 renderModalBody = (
Review comment:
since this isn't a function anymore, I think just `modalBody` is better
##########
File path: superset-frontend/src/SqlLab/components/TemplateParamsEditor.jsx
##########
@@ -68,80 +60,81 @@ 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);
}
- }
+ };
- renderDoc() {
- return (
- <p>
- {t('Assign a set of parameters as')}
- <code>JSON</code>
- {t('below (example:')}
- <code>{'{"my_table": "foo"}'}</code>
- {t('), and they become available in your SQL (example:')}
- <code>SELECT * FROM {'{{ my_table }}'} </code>
- {t(') by using')}
- <a
- href="https://superset.apache.org/sqllab.html#templating-with-jinja"
- target="_blank"
- rel="noopener noreferrer"
- >
- Jinja templating
- </a>{' '}
- syntax.
- </p>
- );
- }
+ useEffect(() => {
+ onChange(codeText);
+ }, [codeText]);
- renderModalBody() {
- return (
- <div>
- {this.renderDoc()}
- <StyledConfigEditor
- keywords={[]}
- mode={this.props.language}
- minLines={25}
- maxLines={50}
- onChange={this.onChange}
- width="100%"
- editorProps={{ $blockScrolling: true }}
- enableLiveAutocompletion
- value={this.state.codeText}
- />
- </div>
- );
- }
+ const renderDoc = (
+ <p>
+ {t('Assign a set of parameters as')}
+ <code>JSON</code>
+ {t('below (example:')}
+ <code>{'{"my_table": "foo"}'}</code>
+ {t('), and they become available in your SQL (example:')}
+ <code>SELECT * FROM {'{{ my_table }}'} </code>
+ {t(') by using')}
Review comment:
optional: the translation string wrappers used here aren't correct because translating chunks of words in a specific order doesn't work in all languages (different languages use different word orders in sentences). I'd recommend removing all the `t` strings here since they're pretty meaningless anyway
##########
File path: superset-frontend/src/SqlLab/components/TemplateParamsEditor.jsx
##########
@@ -68,80 +60,81 @@ 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);
}
- }
+ };
- renderDoc() {
- return (
- <p>
- {t('Assign a set of parameters as')}
- <code>JSON</code>
- {t('below (example:')}
- <code>{'{"my_table": "foo"}'}</code>
- {t('), and they become available in your SQL (example:')}
- <code>SELECT * FROM {'{{ my_table }}'} </code>
- {t(') by using')}
- <a
- href="https://superset.apache.org/sqllab.html#templating-with-jinja"
- target="_blank"
- rel="noopener noreferrer"
- >
- Jinja templating
- </a>{' '}
- syntax.
- </p>
- );
- }
+ useEffect(() => {
+ onChange(codeText);
+ }, [codeText]);
- renderModalBody() {
- return (
- <div>
- {this.renderDoc()}
- <StyledConfigEditor
- keywords={[]}
- mode={this.props.language}
- minLines={25}
- maxLines={50}
- onChange={this.onChange}
- width="100%"
- editorProps={{ $blockScrolling: true }}
- enableLiveAutocompletion
- value={this.state.codeText}
- />
- </div>
- );
- }
+ const renderDoc = (
Review comment:
More of a style nit (and also optional) but I think it's easier to read with this inlined into `modalBody` below. It's not used anywhere else, so there's no real need to pull it out into its own const
----------------------------------------------------------------
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