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/5751 add spider diagram to map #109

Merged
merged 17 commits into from
Aug 2, 2024

Conversation

NekoLyn
Copy link
Contributor

@NekoLyn NekoLyn commented Aug 1, 2024

No description provided.

Copy link
Contributor

@utas-raymondng utas-raymondng left a comment

Choose a reason for hiding this comment

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

Minor comment

@utas-raymondng
Copy link
Contributor

Few more comments but can fixed in next iteration, please raise tickets to capture these and other comments after the end of iteration review

  1. I think we can reduce the zoom level a bit more so we can see the spider earlier
  2. Once the spider appears, if you zoom out then the spider still there but keep reducing, that is spider do not disappear
  3. Very good the color change on end node thanks.

I am ok to approve the PR once the comment inside the code addressed

Copy link
Contributor

@utas-raymondng utas-raymondng left a comment

Choose a reason for hiding this comment

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

Once more minor comment

@NekoLyn
Copy link
Contributor Author

NekoLyn commented Aug 2, 2024

Few more comments but can fixed in next iteration, please raise tickets to capture these and other comments after the end of iteration review

  1. I think we can reduce the zoom level a bit more so we can see the spider earlier
  2. Once the spider appears, if you zoom out then the spider still there but keep reducing, that is spider do not disappear
  3. Very good the color change on end node thanks.

I am ok to approve the PR once the comment inside the code addressed

Thanks Raymond! I agree to spiderify earlier, but it may take some time to adjust the function of calculating the radius of the spider diagram and legs/pins size to scale up/down the diagram when changing the spiderifyFromZoomLevel.

For the second point I will fix it by making the unspiderify more sensitive to the zoom out (for now it will disappear with zoom out but not so quickly) So I will raise another ticket to fix it together with other function/style issues of spider diagram in next iteration.

Copy link
Contributor

@utas-raymondng utas-raymondng left a comment

Choose a reason for hiding this comment

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

LGTM

@utas-raymondng utas-raymondng merged commit 8e15b4a into main Aug 2, 2024
2 checks passed
@utas-raymondng
Copy link
Contributor

Please raise ticket for remain change and put links here thanks

@NekoLyn NekoLyn deleted the feature/5751-add-spider-diagram-to-map branch August 2, 2024 04:37
@NekoLyn
Copy link
Contributor Author

NekoLyn commented Aug 8, 2024

Please raise ticket for remain change and put links here thanks

A ticket to adjust the variable spiderifyFromZoomLevel to make the spiderify appear more earlier https://github.com/aodn/backlog/issues/5810

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