Skip to content
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

Update task creation form #743

Merged
merged 10 commits into from
Aug 24, 2023
Merged

Update task creation form #743

merged 10 commits into from
Aug 24, 2023

Conversation

aaronchongth
Copy link
Member

What's new

  • Refactor delivery task form to be more intuitive
  • Only display dedicated waypoints for patrol, clean and delivery tasks
  • If no waypoints were provided for any of the task, the choice of that task is disabled
  • Migrate word use of Loop to Patrol

Screenshot_20230823_171656

Screenshot_20230823_171712

Self-checks

  • I have prototyped this new feature (if necessary) on Figma
  • I'm familiar with and follow this Typescript guideline
  • I added unit-tests for new components
  • I tried testing edge cases
  • I tested the behavior of the components that interact with the backend, with an e2e test

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
… names

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
…waypoint options only for that type

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #743 (9837319) into main (dec13be) will decrease coverage by 0.14%.
The diff coverage is 14.89%.

@@            Coverage Diff             @@
##             main     #743      +/-   ##
==========================================
- Coverage   54.65%   54.52%   -0.14%     
==========================================
  Files         263      263              
  Lines        6528     6548      +20     
  Branches      862      865       +3     
==========================================
+ Hits         3568     3570       +2     
- Misses       2820     2838      +18     
  Partials      140      140              
Flag Coverage Δ
dashboard 18.37% <40.00%> (-0.08%) ⬇️
react-components 51.77% <8.10%> (-0.35%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...ackages/react-components/lib/tasks/create-task.tsx 2.38% <0.00%> (+0.03%) ⬆️
packages/react-components/lib/place.ts 10.34% <13.63%> (+10.34%) ⬆️
packages/dashboard/src/components/appbar.tsx 31.10% <40.00%> (-0.94%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@Angatupyry Angatupyry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really great!
I just left a small comment that you might consider
The code looks nice too!
Thanks, Aaron!

LGTM!

Comment on lines 39 to 59
for (const level of buildingMap.levels) {
for (const graphs of level.nav_graphs) {
for (const vertex of graphs.vertices) {
if (vertex.name) {
const place: Place = { level: level.name, vertex };
for (const p of vertex.params) {
if (p.name === DEFAULT_PICKUP_POINT_PARAM_NAME) {
pickupPoints.set(vertex.name, place);
}
if (p.name === DEFAULT_DROPOFF_POINT_PARAM_NAME) {
dropoffPoints.set(vertex.name, place);
}
if (p.name === DEFAULT_CLEANING_ZONE_PARAM_NAME) {
cleaningZones.set(vertex.name, place);
}
}
places.set(vertex.name, place);
}
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Maybe we can use destructuring and switch here

Suggested change
for (const level of buildingMap.levels) {
for (const graphs of level.nav_graphs) {
for (const vertex of graphs.vertices) {
if (vertex.name) {
const place: Place = { level: level.name, vertex };
for (const p of vertex.params) {
if (p.name === DEFAULT_PICKUP_POINT_PARAM_NAME) {
pickupPoints.set(vertex.name, place);
}
if (p.name === DEFAULT_DROPOFF_POINT_PARAM_NAME) {
dropoffPoints.set(vertex.name, place);
}
if (p.name === DEFAULT_CLEANING_ZONE_PARAM_NAME) {
cleaningZones.set(vertex.name, place);
}
}
places.set(vertex.name, place);
}
}
}
}
for (const level of buildingMap.levels) {
for (const graphs of level.nav_graphs) {
for (const vertex of graphs.vertices) {
const { name, params } = vertex;
if (!name) {
continue;
}
const place: Place = { level: level.name, vertex };
for (const p of params) {
switch (p.name) {
case DEFAULT_PICKUP_POINT_PARAM_NAME:
pickupPoints.set(name, place);
break;
case DEFAULT_DROPOFF_POINT_PARAM_NAME:
dropoffPoints.set(name, place);
break;
case DEFAULT_CLEANING_ZONE_PARAM_NAME:
cleaningZones.set(name, place);
break;
}
}
places.set(name, place);
}
}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9837319

I've just refactored the if case! Thanks for catching that, @Angatupyry!

Although the repeated if cases look ugly, I think I'll keep it that way since we have no guarantees that a single waypoint can't have multiple of these params (e.g. all 3), also I believe switch statements should be reserved for enums or simple number and chars, I don't think we should be comparing strings that way.

I'll go ahead and merge this now! Thanks!

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth aaronchongth merged commit 1412804 into main Aug 24, 2023
4 checks passed
@aaronchongth aaronchongth deleted the fix/task-form branch August 24, 2023 04:59
This was referenced Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants