-
Notifications
You must be signed in to change notification settings - Fork 13.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: sqleditorleftbar to typescript #17926
refactor: sqleditorleftbar to typescript #17926
Conversation
3e070b1
to
94cc6aa
Compare
id: number; | ||
}; | ||
|
||
interface propTypes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should change this name to be more specific to the component that we are using. Maybe SqlEditorLeftBarProps, which is a mouthful, but I think more specific than what we have currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feed back, I changed the interface to SqlEditorLeftBarProps.
|
||
interface propTypes { | ||
queryEditor: QueryEditor; | ||
height: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both height and tables have a default value, which make me think that they are optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made both of these properties optional👍.
…r different types, still a rough version of final product
94cc6aa
to
2c327a4
Compare
/testenv up |
@AAfghahi Ephemeral environment spinning up at http://54.202.74.189:8080. Credentials are |
} | ||
} | ||
`; | ||
|
||
export default function SqlEditorLeftBar({ | ||
actions, | ||
database, | ||
height, | ||
height = 500, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: put height as the last param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made height the last param👍
Codecov Report
@@ Coverage Diff @@
## master #17926 +/- ##
=======================================
Coverage 66.05% 66.05%
=======================================
Files 1591 1591
Lines 62418 62411 -7
Branches 6286 6289 +3
=======================================
- Hits 41228 41225 -3
+ Misses 19568 19564 -4
Partials 1622 1622
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
height: 500, | ||
offline: false, | ||
tables: [], | ||
type dbType = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason that this is a type over an interface?
&: hover { | ||
color: ${({ theme }) => theme.colors.primary.dark2} !important; | ||
color: ${theme.colors.primary.dark2} !important; | ||
} | ||
} | ||
`; | ||
|
||
export default function SqlEditorLeftBar({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't offline be in here with a default variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offline was removed in my functional conversion PR because the value was never read or used anywhere in the component.
tables?: ExtendedTable[]; | ||
actions: actionsTypes & TableElementProps['actions']; | ||
database: DatabaseObject; | ||
offline: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this has a default value, should it be required?
const onDbChange = db => { | ||
tables = [], | ||
}: SqlEditorLeftBarProps) { | ||
const onDbChange = (db: dbType) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason we are putting in an entire DB in here if we just use the id? WOuld it be better if we passed in just the db.id and then you don't need the DbType type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, will fix that asap.
…torLeftBarProps interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one question, otherwise it looks great! 😁
offline: PropTypes.bool, | ||
}; | ||
interface ExtendedTable extends Table { | ||
expanded: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this able to be described by a type? I remember seeing this with you the other day but don't remember exactly what it was, I think this might have been a string
or a boolean
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to look at a previous commit to see what it was previously. Once I do I'll change it to that type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the expanded type from any to boolean, which is what it was originally.👍
@yousoph and @rosemarie-chiu could you test this? |
/testenv up |
@AAfghahi Ephemeral environment spinning up at http://54.213.205.134:8080. Credentials are |
Ephemeral environment shutdown and build artifacts deleted. |
* Creating draft PR to address bug * Still working on solving re rendering bug * Cleaning up in preparation for push * Starting conversion to TypeScript * Working on conversion * Continued working on typescript conversion, referenced other files for different types, still a rough version of final product * Added type assertion to actions in props, and added types to some component functions * Progress on typescript conversion * Fixed typing issue on collapseStyles * Fixed styling on div, child of StyledScrollbarContainer * Attempting to address issues with the actions passed into the TableElement * Resolved typescript warning on actions of the TableElement component * Made changes suggested by Arash * Tested the component without dbId, cleaned up lingering comments * Made more changes suggested by Arash, removed offline from the SqlEditorLeftBarProps interface * Made change suggested by Hugh * Changed the expanded type from any to boolean
* Creating draft PR to address bug * Still working on solving re rendering bug * Cleaning up in preparation for push * Starting conversion to TypeScript * Working on conversion * Continued working on typescript conversion, referenced other files for different types, still a rough version of final product * Added type assertion to actions in props, and added types to some component functions * Progress on typescript conversion * Fixed typing issue on collapseStyles * Fixed styling on div, child of StyledScrollbarContainer * Attempting to address issues with the actions passed into the TableElement * Resolved typescript warning on actions of the TableElement component * Made changes suggested by Arash * Tested the component without dbId, cleaned up lingering comments * Made more changes suggested by Arash, removed offline from the SqlEditorLeftBarProps interface * Made change suggested by Hugh * Changed the expanded type from any to boolean
* Creating draft PR to address bug * Still working on solving re rendering bug * Cleaning up in preparation for push * Starting conversion to TypeScript * Working on conversion * Continued working on typescript conversion, referenced other files for different types, still a rough version of final product * Added type assertion to actions in props, and added types to some component functions * Progress on typescript conversion * Fixed typing issue on collapseStyles * Fixed styling on div, child of StyledScrollbarContainer * Attempting to address issues with the actions passed into the TableElement * Resolved typescript warning on actions of the TableElement component * Made changes suggested by Arash * Tested the component without dbId, cleaned up lingering comments * Made more changes suggested by Arash, removed offline from the SqlEditorLeftBarProps interface * Made change suggested by Hugh * Changed the expanded type from any to boolean
SUMMARY
Converted the SqlEditorLeftBar component to TypeScript, also want to make note that I removed dbId from being passed into TableSelector.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Should still function as it did before.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION