-
Notifications
You must be signed in to change notification settings - Fork 92
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
Removing texts part b#161 #269
Conversation
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 job 💪
I've made some suggestions
src/App.tsx
Outdated
@@ -62,6 +62,7 @@ const App = () => { | |||
}) | |||
|
|||
useEffect(() => { | |||
// const page = pages.find((page) => page.path === location.pathname) |
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 delete the commented line
@@ -4,6 +4,7 @@ import { PageContainer } from '../components/PageContainer' | |||
import OperatorHbarChart from './OperatorHbarChart/OperatorHbarChart' | |||
import './DashboardPage.scss' | |||
import { TEXTS } from 'src/resources/texts' | |||
// import { useTranslation } from 'react-i18next' |
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 remove
src/routes/index.tsx
Outdated
@@ -24,6 +25,69 @@ import { | |||
LineChartOutlined, | |||
} from '@ant-design/icons' | |||
|
|||
export const getPages = () => { |
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.
export const getPages = () => { | |
export const usePages = () => { |
Every function that calls react hook is a hook
src/routes/index.tsx
Outdated
}, | ||
] | ||
} | ||
|
||
export const PAGES = [ |
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.
Can you delete it?
First I need to remove all references to it, there are still some changes to make to replace the implementations of PAGES with the new usePages hook.
So I'll make the changes and ask again for a review for this PR, one that will complete the removal of TEXT_KEYS from routes.tsx.
Thx a lot for your suggestions, on it:)
src/routes/index.tsx
Outdated
@@ -85,8 +149,9 @@ export const PAGES = [ | |||
] | |||
|
|||
const RoutesList = () => { | |||
const RedirectToDashboard = () => <Navigate to={PAGES[0].path} replace /> | |||
const routes = PAGES.filter((r) => r.element) | |||
const pages = getPages() |
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.
Must be called outside this function, because it's a hook
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.
Must be called outside this function, because it's a hook
If you move usePages outside of a react component, you get this warning:
index.tsx:29 Warning: Invalid hook call. Hooks can only be called inside of the body of a function component.
I think I misunderstood you or something on this one:)
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.
My bad, it should be inside the component, you're right ✅
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.
looks very good to me
מפעילי תח"צ לפי קיום נסיעות מתוכננות
- was something seemed hard to understand
i replaced it by קיום נסיעות
in order to make it more easy understand
but now, i think m- may be it can be better to replace it with
ביצועי תחבורה- מבט כללי
@amabelleS feel free to edit the tests if you want to :) I'll put it in my next PR, changing to "Overview" / "מבט כללי" :) |
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.
nice work!
thanks @amabelleS !
Description
Removed from: GitHubLink & BugReportFormData.
** Next, src>routes.tsx, but we need to implement the PAGES array differently because we can't use a hook outside of a component **
screenshots
GitHubLink
BugReportFormData
EDIT
Meanwhile, I started working on routes.tsx. Pls check the proposal changes I made, and if approved I'll continue.
The idea is to replace the PAGES array with the getPages function:
Also important:
I noticed that the "old" translation of "dashboard_page_title" in text.tsx is: 'מפעילי תח"צ לפי קיום נסיעות מתוכננות'
But the new one in he.json is: 'קיום נסיעות'
Is is on purpose?