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

Feature/add map to robot tab #530

Merged
merged 23 commits into from
Dec 3, 2021
Merged

Feature/add map to robot tab #530

merged 23 commits into from
Dec 3, 2021

Conversation

ChawinTan
Copy link

@ChawinTan ChawinTan commented Oct 21, 2021

What's new

Related to #503 and #504

  • Added schedule visualizer into the robot page tab
  • Swap the positions of the battery and task progress
  • Add zooming and centering when a robot is clicked

There is a minor inconsistency currently in the zooming feature. When a robot is assigned a task and on the move, you would need to click on the refresh button on the panel to get the latest position of the robot before clicking on the robot row, otherwise the map would center to its current position when the robot has already move away.

updated-screenshot-robot-tab

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

Discussion

ChawinTan added 5 commits October 19, 2021 15:12
Signed-off-by: ChawinTan <chawin15@gmail.com>
Signed-off-by: ChawinTan <chawin15@gmail.com>
Signed-off-by: ChawinTan <chawin15@gmail.com>
Signed-off-by: ChawinTan <chawin15@gmail.com>
Signed-off-by: ChawinTan <chawin15@gmail.com>
@codecov
Copy link

codecov bot commented Oct 21, 2021

Codecov Report

Merging #530 (409d6a3) into main (da74cf3) will decrease coverage by 0.15%.
The diff coverage is 54.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #530      +/-   ##
==========================================
- Coverage   80.57%   80.41%   -0.16%     
==========================================
  Files         255      256       +1     
  Lines        5384     5428      +44     
  Branches      601      599       -2     
==========================================
+ Hits         4338     4365      +27     
- Misses        849      865      +16     
- Partials      197      198       +1     
Flag Coverage Δ
dashboard 70.13% <48.78%> (-1.39%) ⬇️
react-components 77.26% <80.00%> (+0.20%) ⬆️
reporting 41.95% <ø> (ø)

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

Impacted Files Coverage Δ
...ckages/react-components/lib/robots/robot-table.tsx 85.71% <ø> (ø)
...ackages/dashboard/src/util/common-subscriptions.ts 48.78% <48.78%> (ø)
...ackages/react-components/lib/robots/robot-info.tsx 81.81% <66.66%> (+1.81%) ⬆️
...ckages/react-components/lib/robots/robot-panel.tsx 87.50% <83.33%> (+19.07%) ⬆️
packages/react-components/lib/map/map.tsx 66.66% <100.00%> (-1.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da74cf3...409d6a3. Read the comment docs.

ChawinTan added 2 commits October 21, 2021 14:53
Signed-off-by: ChawinTan <chawin15@gmail.com>
Signed-off-by: ChawinTan <chawin15@gmail.com>
ChawinTan added 3 commits October 25, 2021 15:08
Signed-off-by: ChawinTan <chawin15@gmail.com>
Signed-off-by: ChawinTan <chawin15@gmail.com>
@ChawinTan ChawinTan marked this pull request as ready for review October 27, 2021 03:03
React.useEffect(() => {
if (!leaflet.map) return;
if (zoom) leaflet.map.setZoom(zoom);
Copy link
Author

Choose a reason for hiding this comment

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

I increase the zoom value because the original zoom is really small.

@ChawinTan ChawinTan requested review from koonpeng and vallq October 27, 2021 03:09
Copy link
Contributor

@vallq vallq left a comment

Choose a reason for hiding this comment

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

robot.centering.of.icon.mov

I'm not sure if it's just on my end but there seem to be 2 different panning motions (1 immediate, 1 delayed) when there are no active tasks. if robot1 is currently centered, clicking on robot2 moves the position immediately (no panning motion) but if there are no robots in focus/center, clicking on either entry will show the desired panning motion
is this something that can be kept the same?

also could the mouse icon change to a suitable icon that indicates the entries are clickable?


// robot panel stuff
const [hasMore, setHasMore] = React.useState(true);
const [page, setPage] = React.useState(0);
const [verboseRobots, setVerboseRobots] = React.useState<VerboseRobot[]>([]);
const fetchVerboseRobots = React.useCallback(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

would it help if the fetching of the robots states can be toggled with autorefresh like the tasks tabs to fix the problem of the position og the centering feature?

Copy link
Author

Choose a reason for hiding this comment

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

Sure! Maybe I could create a separate task for this.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed the positioning issue by calling fetchVerboseRobots when clicking the robot.

@ChawinTan
Copy link
Author

ChawinTan commented Nov 2, 2021

I'm not sure if it's just on my end but there seem to be 2 different panning motions (1 immediate, 1 delayed) when there are no active tasks. if robot1 is currently centered, clicking on robot2 moves the position immediately (no panning motion) but if there are no robots in focus/center, clicking on either entry will show the desired panning motion
is this something that can be kept the same?

This will require some digging into the behavior of how setView works. Might take a while.

also could the mouse icon change to a suitable icon that indicates the entries are clickable?

Thanks for pointing that out!

Signed-off-by: ChawinTan <chawin15@gmail.com>
@ChawinTan
Copy link
Author

Turns out the panning motion can be fixed by simply setting the animation option in setView to true.

Signed-off-by: ChawinTan <chawin15@gmail.com>
ChawinTan added 2 commits November 5, 2021 10:30
Signed-off-by: ChawinTan <chawin15@gmail.com>
Signed-off-by: ChawinTan <chawin15@gmail.com>
packages/dashboard/src/util/common-subscriptions.ts Outdated Show resolved Hide resolved
packages/dashboard/src/util/common-subscriptions.ts Outdated Show resolved Hide resolved
packages/react-components/lib/map/map.tsx Outdated Show resolved Hide resolved
packages/react-components/lib/map/map.tsx Outdated Show resolved Hide resolved
packages/react-components/lib/robots/robot-panel.spec.tsx Outdated Show resolved Hide resolved
packages/react-components/lib/robots/robot-panel.tsx Outdated Show resolved Hide resolved
ChawinTan added 3 commits November 15, 2021 17:38
Signed-off-by: ChawinTan <chawin15@gmail.com>
Signed-off-by: ChawinTan <chawin15@gmail.com>
Signed-off-by: ChawinTan <chawin15@gmail.com>
@ChawinTan
Copy link
Author

  • Removed all the setLeafletMap and associated props and exported the map from schedule visualizer instead
  • other changes in unit test and changing function to hooks

@ChawinTan ChawinTan requested a review from koonpeng November 16, 2021 02:08
ChawinTan added 2 commits November 23, 2021 09:57
Signed-off-by: ChawinTan <chawin15@gmail.com>
Signed-off-by: ChawinTan <chawin15@gmail.com>
ChawinTan added 3 commits November 30, 2021 08:34
Signed-off-by: ChawinTan <chawin15@gmail.com>
Signed-off-by: ChawinTan <chawin15@gmail.com>
Signed-off-by: ChawinTan <chawin15@gmail.com>
@ChawinTan ChawinTan requested a review from koonpeng November 30, 2021 07:32
Signed-off-by: ChawinTan <chawin15@gmail.com>
@ChawinTan ChawinTan requested a review from koonpeng December 3, 2021 08:49
@ChawinTan ChawinTan merged commit a0d2182 into main Dec 3, 2021
@ChawinTan ChawinTan deleted the feature/add-map-to-robot-tab branch December 3, 2021 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants