[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10529: fix: misaligned LimitControl buttons and port jsx->tsx

[GitBox <gi...@apache.org>]

etr2460 commented on a change in pull request #10529:
URL: https://github.com/apache/incubator-superset/pull/10529#discussion_r466488756



##########
File path: superset-frontend/src/SqlLab/components/LimitControl.tsx
##########
@@ -30,15 +29,27 @@ import { t } from '@superset-ui/translation';
 
 import ControlHeader from '../../explore/components/ControlHeader';
 
-const propTypes = {
-  value: PropTypes.number,
-  defaultQueryLimit: PropTypes.number.isRequired,
-  maxRow: PropTypes.number.isRequired,
-  onChange: PropTypes.func.isRequired,
+export interface LimitControlProps {
+  value?: number;
+  defaultQueryLimit: number;
+  maxRow: number;
+  onChange: (value: number) => void;
+}
+
+interface LimitControlState {
+  textValue: string;
+  showOverlay: boolean;
+}
+
+const popoverStyle = {

Review comment:
       instead of this, let's make a `PopoverDiv` component with `styled` and apply the styling that way?

##########
File path: superset-frontend/src/SqlLab/components/LimitControl.tsx
##########
@@ -99,18 +110,21 @@ export default class LimitControl extends React.PureComponent {
               value={textValue}
               placeholder={t(`Max: ${this.props.maxRow}`)}
               bsSize="small"
-              onChange={e => this.setState({ textValue: e.target.value })}
+              // @ts-ignore

Review comment:
       Why do we need the ts ignore here?

##########
File path: superset-frontend/src/SqlLab/components/LimitControl.tsx
##########
@@ -99,18 +110,21 @@ export default class LimitControl extends React.PureComponent {
               value={textValue}
               placeholder={t(`Max: ${this.props.maxRow}`)}

Review comment:
       while you're in here, wanna fix the translation string here too?

##########
File path: superset-frontend/src/SqlLab/components/LimitControl.tsx
##########
@@ -88,7 +99,7 @@ export default class LimitControl extends React.PureComponent {
         : '');
     return (
       <Popover id="sqllab-limit-results">
-        <div style={{ width: '100px' }}>
+        <div style={popoverStyle}>
           <ControlHeader
             label={t('Row limit')}
             validationErrors={!isValid ? [t(errorMsg)] : []}

Review comment:
       The translation function here probably doesn't do anything




----------------------------------------------------------------
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