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

fix alignLeftAt: alignRightAt: to better fit the implementation of Vertical Alignment #381

Merged
merged 5 commits into from
Jan 23, 2024

Conversation

Enzo-Demeulenaere
Copy link
Contributor

I spotted this issue while in a call with @tinchodias, where in the BlClickEventExamplesTest>>threeElementsWithFalseChildClipping, the placement of elements would not feel right when reading alignRightAt: and alignTopAt: .

I noticed that Vertical Alignment used "BlVerticalCoordinateTopAlignment" and "BlVerticalCoordinateBottomAlignment" while Horizontal Alignment used only "BlHorizontalCoordinateStartAlignment" (while also not having any method alignLeftAt: when dealing with ignored layout)

I propose this fix that matches the implementation of Vertical alignment, describing my methods as such :

alignLeftAt: Align the left of the element horizontally with a unit-coordinate inside of the parent, going from left to right

alignRightAt: "Align the right of the element horizontally with a unit-coordinate inside of the parent, going from right to left"

Copy link
Collaborator

@tinchodias tinchodias left a comment

Choose a reason for hiding this comment

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

@Enzo-Demeulenaere thanks for the change.

Maybe you can add some test? I didn't check where should it be. I guess in the frame layout tests, and ignore layout tests?

What do you think?

Comment on lines 36 to 38
alignment := BlHorizontalCoordinateStartAlignment new
alignment := BlHorizontalCoordinateLeftAlignment new
coordinate: aUnitCoordinateNumber;
relativeAlignment: BlElementAlignment horizontal start
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @Enzo-Demeulenaere doesn't it work keeping BlHorizontalCoordinateStartAlignment instead of BlHorizontalCoordinateLeftAlignment in this case?

Maybe @plantec has some view on this, we discussed something related to start/end left/right a time ago...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,

By looking at the code I would say this doesn't change anything as the coordinate: and relativeAlignment: methods are defined and called in the 'BlHorizontalCoordinateAlignment' superclass

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused with this code (not the change but the whole code). This "aUnitCoordinateNumber" is probably a [0..1] ratio. I find strange the term coordinate, or am I wrong?

Comment on lines -55 to 57
alignment := BlHorizontalCoordinateStartAlignment new
alignment := BlHorizontalCoordinateRightAlignment new
coordinate: aUnitCoordinateNumber;
relativeAlignment: BlElementAlignment horizontal end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow, I see that evaluation of BlElementAlignment horizontal end returns an instance of BlHorizontalEndAlignment.

So, we were creating here an instance of BlHorizontalCoordinateStartAlignment that has a "relative alignment" of BlHorizontalEndAlignment... a horizontal coordinate start with a relative horizontal end alignment... it sounds redundant and contradictory at the same time. I'm lost of what concept is this representing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean but now that we have a RightAlignment with an EndAlignment does this mean it goes from right to left ? or just from right to right somehow ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess that if a layout were in rightToLeft mode, then start means right and end means left. But in the most usual leftToRight mode yes, start=left and end=right. But not sure how is it working.

@Enzo-Demeulenaere
Copy link
Contributor Author

Enzo-Demeulenaere commented Jan 23, 2024

Hi @tinchodias I just added tests as well as a small cheat sheet (in Bloc-Examples) for anyone to see it when trying to understand align[Direction]At:

Here's how it looks
Capture d’écran 2024-01-23 à 10 09 02

@tinchodias
Copy link
Collaborator

looks great thanks

@tinchodias tinchodias merged commit f9fc310 into pharo-graphics:dev-1.0 Jan 23, 2024
6 checks passed
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