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
##########
File path: superset-frontend/src/SqlLab/components/TemplateParamsEditor.jsx
##########
@@ -68,23 +60,27 @@ 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 !== code) {
+ onChange(newValue);
}
- }
+ };
+
+ useEffect(() => {
+ onChange(codeText);
+ }, [codeText]);
- renderDoc() {
- return (
+ const modalBody = (
+ <div>
<p>
- {t('Assign a set of parameters as')}
+ 'Assign a set of parameters as'
Review comment:
These probably shouldn't have quotes here anymore. Did you validate the
change to make sure the test renders properly?
##########
File path: superset-frontend/src/SqlLab/components/TemplateParamsEditor.jsx
##########
@@ -68,23 +60,27 @@ 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 !== code) {
+ onChange(newValue);
}
- }
+ };
+
+ useEffect(() => {
+ onChange(codeText);
+ }, [codeText]);
- renderDoc() {
- return (
+ const modalBody = (
+ <div>
<p>
- {t('Assign a set of parameters as')}
+ 'Assign a set of parameters as'
Review comment:
lol, this was a typo, i meant to say "make sure the _text_ renders
properly"
##########
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:
woah, when did this happen? I could've sworn this wasn't the case....
cc @williaster who I think told me this before too
##########
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:
Seems like I misunderstood in the past, it's only if you directly
`export default` a function where this becomes an issue. Whichever folks prefer
should be fine here, not sure if we want to have a standard best practice for
this type of stuff in superset or not (maybe there's a lint rule we could use
to enforce consistency, one way or another)
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
