-
Notifications
You must be signed in to change notification settings - Fork 3
pos-filter-ui #366
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
base: main
Are you sure you want to change the base?
pos-filter-ui #366
Changes from all commits
f627fca
d12ba73
a665a97
4c2279c
18e5b85
9544c7a
8e7d4d4
df7b91d
1986394
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. eniig baiikhgu bolgoh |
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. eniig ch bas |
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. eniig bas |
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ene ter chigeere ustaagui yu |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import { atom } from 'jotai'; | ||
|
|
||
| export const posTotalCountAtom = atom<number | null>(null); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,11 +6,17 @@ import { | |
| import { lazy, Suspense } from 'react'; | ||
|
|
||
| const SalesNavigation = lazy(() => | ||
| import('./modules/SalesNavigation').then((module) => ({ | ||
| import('./modules/navigation/SalesNavigation').then((module) => ({ | ||
| default: module.SalesNavigation, | ||
| })), | ||
| ); | ||
|
|
||
| const PosOrderNavigation = lazy(() => | ||
| import('./modules/navigation/PosOrderNavigation').then((module) => ({ | ||
| default: module.PosOrderNavigation, | ||
| })), | ||
| ); | ||
|
|
||
| export const CONFIG = { | ||
| name: 'sales', | ||
| icon: IconBriefcase, | ||
|
|
@@ -24,20 +30,28 @@ export const CONFIG = { | |
| <SalesNavigation /> | ||
| </Suspense> | ||
| ), | ||
| subGroups: () => ( | ||
| <Suspense fallback={<div />}> | ||
| <PosOrderNavigation /> | ||
| </Suspense> | ||
| ), | ||
| }, | ||
| modules: [ | ||
| { | ||
| name: 'deals', | ||
| icon: IconSandbox, | ||
| path: 'deals', | ||
| hasSettings: true, | ||
| name: 'sales', | ||
| icon: IconBriefcase, | ||
| path: 'sales', | ||
| hasSettings: false, | ||
| hasRelationWidget: true, | ||
| hasFloatingWidget: false, | ||
|
Comment on lines
+41
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainVerify breaking changes: module rename and disabled settings. This segment contains potentially breaking changes:
Ensure these changes are intentional and that:
Run the following script to verify references to the old module name and settings usage: 🏁 Script executed: #!/bin/bash
# Search for references to 'deals' module that might break
rg -nP --type=ts --type=tsx -C3 "['\"](deals)['\"]|/deals\b" -g '!frontend/plugins/sales_ui/src/config.tsx'
# Search for hasSettings usage to understand impact
rg -nP --type=ts --type=tsx -C3 'hasSettings' -g '!frontend/plugins/sales_ui/src/config.tsx'Length of output: 55 🏁 Script executed: #!/bin/bash
# Search for references to 'deals' module or route outside config.tsx
rg -nP -C3 "(\"deals\"|'deals')|/deals\b" -g '!frontend/plugins/sales_ui/src/config.tsx'
# Search for any usage of hasSettings outside config.tsx
rg -nP -C3 'hasSettings' -g '!frontend/plugins/sales_ui/src/config.tsx'Length of output: 297 🏁 Script executed: #!/bin/bash
# Unfiltered search for references to 'deals' module or route
rg -nP -C3 "(\"deals\"|'deals')|/deals\b"
# Unfiltered search for any usage of hasSettings
rg -nP -C3 'hasSettings'Length of output: 106095 Update all references after renaming ‘deals’ to ‘sales’ and disabling settings
🤖 Prompt for AI Agents |
||
| }, | ||
| { | ||
| name: 'pos', | ||
| icon: IconCashRegister, | ||
| path: 'pos', | ||
| hasSettings: true, | ||
| hasSettings: false, | ||
| hasRelationWidget: true, | ||
| hasFloatingWidget: false, | ||
| }, | ||
| ], | ||
| }; | ||
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import { Navigate, Route, Routes } from 'react-router'; | ||
| import { Suspense, lazy } from 'react'; | ||
| import { Spinner } from 'erxes-ui/components'; | ||
|
|
||
| const DealsMain = lazy(() => | ||
| import('~/pages/SalesIndexPage').then((module) => ({ | ||
| default: module.SalesIndexPage, | ||
| })), | ||
| ); | ||
|
|
||
| const OrdersMain = lazy(() => | ||
| import('~/pages/OrdersPage').then((module) => ({ | ||
| default: module.OrdersPage, | ||
| })), | ||
| ); | ||
|
|
||
| const PosMain = lazy(() => | ||
| import('~/pages/PosIndexPage').then((module) => ({ | ||
| default: module.PosIndexPage, | ||
| })), | ||
| ); | ||
|
|
||
| const App = () => { | ||
| return ( | ||
| <Suspense fallback={<Spinner />}> | ||
| <Routes> | ||
| <Route path="/" element={<Navigate to="deals" replace />} /> | ||
| <Route path="/deals" element={<DealsMain />} /> | ||
| <Route path="/pos" element={<Navigate to="orders" replace />} /> | ||
| <Route path="/pos/:posId/orders" element={<OrdersMain />} /> | ||
|
Comment on lines
+29
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redirect is missing the required Navigating to
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. eniig harsan uu There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @KhBaterdene Yes, I've reviewed this route configuration. The issue I flagged is still present in the current code: The Problem: Suggested Fix:
<Route path="/pos" element={<PosRedirect />} />Where
<Route path="/pos" element={<Navigate to="/deals" replace />} />Redirect to deals instead if there's no default POS
<Route path="/pos/orders" element={<OrdersMain />} />
<Route path="/pos/:posId/orders" element={<OrdersMain />} />And handle the missing Which approach would work best for your use case? |
||
| <Route path="/pos/:posId" element={<PosMain />} /> | ||
| </Routes> | ||
| </Suspense> | ||
| ); | ||
| }; | ||
|
|
||
| export default App; | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify trailing slash handling in API URL usage.
The trailing slash added to
REACT_APP_API_URLcould cause double-slash issues (e.g.,http://localhost:4000//api/endpoint) if the code concatenates paths without accounting for it. Ensure all API calls handle this correctly.Run the following script to check how the API URL is used throughout the codebase:
🏁 Script executed:
Length of output: 55
🏁 Script executed:
Length of output: 16087
Normalize or remove trailing slash in REACT_APP_API_URL
.env.samplecurrently definesREACT_APP_API_URL=http://localhost:4000/, which leads to URLs likehttp://localhost:4000//upload-filewhen concatenated. Either drop the trailing slash in.env.sampleor strip it at runtime (e.g.envApiUrl.replace(/\/$/, '')) before appending paths.🤖 Prompt for AI Agents
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.
ene odoo yajiinaa mi hu
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.
ene odoo yajiinaa mi hu
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.
@KhBaterdene Thank you for confirming! If you've verified that the trailing slash works correctly with your API endpoint handling, that's great. The change looks good to proceed.
Just for future reference, if you notice any double-slash issues in API URLs (like
http://localhost:4000//api/...), you'll know the trailing slash in the.envfile is the place to adjust. ✨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.
@KhBaterdene Glad to hear it's working well now! 🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!