refactor: migrate SqlEditor component to FC & tsx #18148
Conversation
There was a problem hiding this comment.
Hey @EugeneTorap thanks for the work that you put into this! Please find below some initial comments from a first look.
| import { | ||
| Dropdown, | ||
| Menu as AntdMenu, | ||
| Menu, | ||
| Switch, | ||
| Input, | ||
| } from 'src/common/components'; |
There was a problem hiding this comment.
The components Dropdown and Switch are available in the src/components directory. I think we should prefer those to keep consistency.
There was a problem hiding this comment.
@geido Dropdown UI is changed
There was a problem hiding this comment.
I think I should revert these changes
| actions: object; | ||
| database?: DatabaseObject; | ||
| latestQuery?: Query; | ||
| tables: any[]; | ||
| editorQueries: QueryEditor[]; | ||
| dataPreviewQueries: object[]; | ||
| queryEditorId: string; | ||
| hideLeftBar?: boolean; | ||
| defaultQueryLimit: number; | ||
| maxRow: number; | ||
| displayLimit: number; | ||
| saveQueryWarning?: string; | ||
| scheduleQueryWarning?: string; |
There was a problem hiding this comment.
Do you think that for actions tables dataPreviewQueries we can be a little more specific for their types?
There was a problem hiding this comment.
It's better do in next PR
There was a problem hiding this comment.
@geido Do we have interface for all sqlLab actions & dataPreviewQueries?
There was a problem hiding this comment.
@EugeneTorap if you can't find any reference in the code, we probably don't have but I am happy to do that in a related PR!
| const sqlEditorRef = useRef(null); | ||
| const northPaneRef = useRef(null); | ||
|
|
||
| useState(() => { |
There was a problem hiding this comment.
Are you sure that you wanted to use useState here?
There was a problem hiding this comment.
This useState for UNSAFE_componentWillMount replacement
| <Menu onClick={this.handleMenuClick} style={{ width: 176 }}> | ||
| <Menu.Item style={{ display: 'flex', justifyContent: 'space-between' }}> |
There was a problem hiding this comment.
We should use emotion for styling both Menu and Menu.Item and avoid inline styles wherever possible
|
I am now running the CI. WE should spin up a test env as soon as it passes |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue |
SUMMARY
Migrated SqlEditor to TypeScript to apply direction outlined in #18100 .
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION