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 a new control AspectRatioLayout #480

Merged
merged 4 commits into from
Nov 14, 2024
Merged

Conversation

WCKYWCKF
Copy link
Contributor

The control AspectRatioLayout provides a feature to select control layout based on the aspect ratio of the visual area (Bounds).

@rabbitism
Copy link
Member

Go to AspectRatioLayout demo and go to another and go back, the demo will crash.

@WCKYWCKF
Copy link
Contributor Author

Providing a default value in AspectRatioLayout.ItemsProperty causes all AspectRatioLayouts to share a List. This issue has been fixed, please help review it.

@rabbitism
Copy link
Member

rabbitism commented Nov 13, 2024

My suggestions:
the AspectRatioMode concept kinda limit extensibility. I would recommend introduce something like

public double MinRatio { get; set; }
public double MaxRaio { get; set; }

for layout item.

when bounds ratio change, find the first item that fall into it's range.
Any thoughts?

@WCKYWCKF
Copy link
Contributor Author

My suggestions: the AspectRatioMode concept kinda limit extensibility. I would recommend introduce something like

public double MinRatio { get; set; }
public double MaxRaio { get; set; }

for layout item.

when bounds ratio change, find the first item that fall into it's range. Any thoughts?

Having users study the relationship between aspect ratio values and shapes can degrade the development experience.
It might be a good idea to apply the relationship between Items and ItemsSource to your suggestion.

@WCKYWCKF
Copy link
Contributor Author

Features have been added, please help review.

@rabbitism
Copy link
Member

I asked github copilot, it suggest to rename AspectRatioChangeAmbiguity to AspectRatioTolerance

AvaloniaProperty.Register<AspectRatioLayout, double>(
nameof(AspectRatioChangeAmbiguity), 0.2);

public static readonly StyledProperty<AspectRatioMode> CurrentAspectRatioModeProperty =
Copy link
Member

Choose a reason for hiding this comment

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

Should be a readonly DirectProperty.

set => SetValue(AspectRatioChangeAmbiguityProperty, value);
}

private void UpdataHistory(bool value)
Copy link
Member

Choose a reason for hiding this comment

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

typo: Updata -> Update


private bool _isUseAspectRatioRange;

public static readonly DirectProperty<AspectRatioLayoutItem, bool> IsUseAspectRatioRangeProperty =
Copy link
Member

Choose a reason for hiding this comment

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

An internal CLR property is enough?

Copy link
Member

@rabbitism rabbitism left a comment

Choose a reason for hiding this comment

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

Functionally works well, please check these minor issues.

2.CurrentAspectRatioModeProperty to DirectProperty.
3.Fix misspelling: Updata -> Update
4.IsUseAspectRatioRange to an internal CLR property.
@WCKYWCKF
Copy link
Contributor Author

Four minor issues have been fixed.

@rabbitism rabbitism merged commit 8fe15b2 into irihitech:main Nov 14, 2024
2 checks passed
@rabbitism rabbitism added this to the Nov Release milestone Nov 17, 2024
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