-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
Re-architecture the "TaskList" entity in the backend #4267
Conversation
b36c506
to
177d402
Compare
4360fb4
to
089ce11
Compare
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.
Request changes
is only due to the hook usage, all the rest are just suggestions
I guess with such amount of changes we should probably start releasing it with smaller instances; also checking if nothing breaks accidentally in the mobile app
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 it TourTest
now?
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.
not yet
actually TaskCollection
(ordered set of tasks, which has a distance
and polyline
) is an abstract class/base class that has now two implementations Delivery
and Tours
(TaskList
was but not anymore).
for your information we use doctrine inheritance mapping to translate this abstraction to the database-level.
(I am not really fond of this both on the code side or database side as it adds complexity to the understanding of both the code and the codebase, but it is like this for now)
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.
In my opinion it's better to avoid such generic subscribers and have specialized classes instead, where each class has more focused set of actions/responsibilities. See Domain\Order\Reactor
package, for example
js/app/dashboard/components/Tour.js
Outdated
const Tour = ({ tour, tourId, draggableIndex }) => { | ||
|
||
if (!tour && tourId) { | ||
tour = useSelector(state => selectTourById(state, tourId)) |
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.
Hooks should not be called conditionally: https://react.dev/reference/rules/rules-of-hooks#only-call-hooks-at-the-top-level
Solves also #4007