-
Notifications
You must be signed in to change notification settings - Fork 6
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
[DYN-6868] migrate to typeScript #24
[DYN-6868] migrate to typeScript #24
Conversation
package.json
Outdated
@@ -3,16 +3,16 @@ | |||
"version": "1.0.15", |
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.
please bump up version always
@Enzo707 Did you get the chance to fix the unit tests? |
src/components/LayoutContainer.tsx
Outdated
import { Sidebar } from './Sidebar/Sidebar.jsx'; | ||
import { useState } from 'react'; | ||
import { MainContent } from '../components/MainContent'; | ||
import { Sidebar } from '../components/Sidebar/Sidebar'; |
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.
Cant we import from the same folder?
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.
Sure, we can also implement an index file and import everything from feature folder or just from /components root:
import { MainContent } from './MainContent';
import { Sidebar } from './Sidebar';
Or
import { MainContent, Sidebar } from './';
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.
Thanks, @Enzo707 Would you update? I would like the import statements to be consistent
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.
@QilongTang I already updated the imports. PTAL
src/components/MainContent.tsx
Outdated
import { useState, useEffect } from 'react'; | ||
import { RecentPage } from '../components/Recent/PageRecent'; | ||
import { SamplesPage } from '../components/Samples/PageSamples'; | ||
import { LearningPage } from '../components/Learning/PageLearning'; |
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.
same question here
import { GridViewIcon, ListViewIcon } from '../Common/CustomIcons.jsx'; | ||
import { openFile, saveHomePageSettings } from '../../functions/utility.js'; | ||
import { GraphGridItem } from './GraphGridItem'; | ||
import { CustomNameCellRenderer } from './CustomNameCellRenderer'; |
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 are able to import from the folder here?
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.
LGTM with couple questions
This PR is for migrating DynamoHome from js to typeScript as a first step before adding test coverage.
Ref.: DYN-6868
Review
@QilongTang
@dnenov