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

Add schWidth prop with a test #87

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

Add schWidth prop with a test #87

wants to merge 4 commits into from

Conversation

Slaviiiii
Copy link

No description provided.

@Slaviiiii Slaviiiii requested a review from seveibar July 27, 2024 14:23
Copy link
Contributor

@seveibar seveibar left a comment

Choose a reason for hiding this comment

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

this is a pretty good PR, but i think it would be better to modify getPortArrangementSize to support taking a schWidth prop, transforming things can make the code more complicated over time since it's not clear where the original width is.

@Slaviiiii Slaviiiii requested a review from seveibar July 29, 2024 15:27
Copy link
Contributor

@seveibar seveibar left a comment

Choose a reason for hiding this comment

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

I dont see how this code could work, you should show it worked in the test. Also there are a lot of unnecessary changes or changes to the API (schWidth should be top level, not inside of portArrangement)

There was no math to compute the width as far as i could see so i dont know if this would work? Maybe show in the test that it worked

schPortArrangement: {
leftSize: 4,
rightSize: 4,
schWidth: 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Isnt this supposed the be a top level prop? Why is it inside portArrangement?

Copy link
Author

Choose a reason for hiding this comment

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

this is a pretty good PR, but i think it would be better to modify getPortArrangementSize to support taking a schWidth prop, transforming things can make the code more complicated over time since it's not clear where the original width is.

I thought you meant that I needed to add it there.

Copy link
Author

@Slaviiiii Slaviiiii Jul 29, 2024

Choose a reason for hiding this comment

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

Here is the bug with schWidth set to 2

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea it looks broken, the ports should be outside of the box

Copy link
Author

Choose a reason for hiding this comment

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

They are like glued - the box changes its width, but they do not move

image

)
.build()

await logSoup(soup)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test for the schWidth

@Slaviiiii
Copy link
Author

Slaviiiii commented Jul 29, 2024

@seveibar from what I see here the only place that will get the schWidth prop away from the original width is the source object, right?

image

Copy link

@Anshgrover23 Anshgrover23 left a comment

Choose a reason for hiding this comment

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

@Slaviiiii checks are failing. kindly run npm run format

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.

4 participants