Skip to content

Place nodes on a horizontal line rather than randomly#13

Merged
loichuder merged 1 commit into
mainfrom
fix-pos
Jun 2, 2026
Merged

Place nodes on a horizontal line rather than randomly#13
loichuder merged 1 commit into
mainfrom
fix-pos

Conversation

@loichuder

Copy link
Copy Markdown
Member

Ex for triangle1:

Before:
image

After:
image

@codecov

codecov Bot commented May 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@loichuder loichuder force-pushed the remove-cli branch 2 times, most recently from e324ef5 to 707a414 Compare June 2, 2026 07:28
Base automatically changed from remove-cli to main June 2, 2026 07:28
@loichuder loichuder marked this pull request as ready for review June 2, 2026 09:27
@loichuder loichuder requested a review from LudoBroche June 2, 2026 09:27
@@ -117,3 +110,17 @@
x2=self._box.width,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
x2=self._box.width,
x2=self.width,

self.height = height
self.elements: List[Union[SvgElement, SvgGroup]] = []

def add_background(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the current canvas definition.
We are force to have the following sequence:

  1. Elements creation
  2. Elements Placement
  3. Compute Canvas size
  4. Create Canvas
  5. Create Canvas background
  6. Add elements to Canvas

It's a little counter intuitive to "draw" first and paste into canvas after.
Why not :

  1. Create Canvas (0 size or random)
  2. Create and add elements to canvas
  3. Place elements
  4. Resize background

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hm, I see.

If you agree though, I would like to first implement the complete SVG generation (with edges) before changing this. New abstractions may emerge so that the sequence may more intuitive or be more easily changed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sound good to me

Comment on lines 90 to 91
self._outputs.translate(x=self._box.width)
self._title.set_position(x=self._box.width / 2.0)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
self._outputs.translate(x=self.width)
self._title.set_position(x=self.width / 2.0)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Personally, I avoid using @property getters in the internal methods.

An internal method can have access to internal members and it removes an indirection layer.

@loichuder loichuder merged commit d7343ca into main Jun 2, 2026
6 checks passed
@loichuder loichuder deleted the fix-pos branch June 2, 2026 13:30
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