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

#916 Free drag and magnetic layout #917

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

#916 Free drag and magnetic layout #917

wants to merge 8 commits into from

Conversation

langonginc
Copy link
Member

No description provided.

@thekingofcity thekingofcity linked an issue Jan 5, 2025 that may be closed by this pull request
@thekingofcity thekingofcity marked this pull request as ready for review January 5, 2025 07:57
Copy link
Member

@thekingofcity thekingofcity left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -252,6 +327,18 @@ const SvgCanvas = () => {

const SingleColor = singleColor.component;

const getPolylinesPath = (p: Polyline): [number, number, number, number] => {
if (p.a === 0) {
return [-10000, 10000, -p.c / p.b, -p.c / p.b];
Copy link
Member

@thekingofcity thekingofcity Jan 7, 2025

Choose a reason for hiding this comment

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

If the station and/or user are looking on is beyond 10000, what will happen? Will the line be missing?

Copy link
Member

@thekingofcity thekingofcity left a comment

Choose a reason for hiding this comment

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

Would be better if the comments are refined

{activePolylines.length !== 0 &&
activePolylines.map((p, i) => (
<path
key={`line_polyline_${i}`}
Copy link
Member

Choose a reason for hiding this comment

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

We should use the a,b,c,x,y in p instead of i

https://robinpokorny.com/blog/index-as-a-key-is-an-anti-pattern/


if (isNodeSupportPolyline(node, graph.current)) {
if (activePolylines.length !== 0) {
// remove the active polylines if the distance is larger than 5
Copy link
Member

Choose a reason for hiding this comment

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

This comment tells the same thing the next line of code tells. Comments should be the reason why we do this not what we are doing. e.g. Remove some polylines if they are far away from the targeting position as the user is moving the station to a new position.

https://stackoverflow.blog/2021/12/23/best-practices-for-writing-code-comments/

setActivePolylines(activePolylines.filter(l => getPolylineDistance(l, toX, toY) <= 5));
}
if (activePolylines.length < 2) {
// add the nearest polylines if the distance is less than 3
Copy link
Member

Choose a reason for hiding this comment

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

This comment tells the same thing the next line of code tells. Comments should be the reason why we do this not what we are doing.

https://stackoverflow.blog/2021/12/23/best-practices-for-writing-code-comments/

const toX = fromX - ((offset.x - x) * svgViewBoxZoom) / 100;
const toY = fromY - ((offset.y - y) * svgViewBoxZoom) / 100;

let newX = toX;
Copy link
Member

Choose a reason for hiding this comment

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

Simply explaining what toXY and newXY are will help in understanding the code.

y: number;
}

export interface Polylines {
Copy link
Member

Choose a reason for hiding this comment

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

c, p, n are hard to understand

return Math.abs(line.a * x + line.b * y + line.c) / Math.sqrt(line.a ** 2 + line.b ** 2);
};

export const getNearestPolyline = (x: number, y: number, polylines: Polyline[], nodes: Id[]) => {
Copy link
Member

Choose a reason for hiding this comment

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

Simply explain how we find the nearest polyline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Free drag and magnetic layout
2 participants