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

Grid: RowDefinition and ColumnDefinition Constructor changes #85

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

anawishnoff
Copy link
Contributor

No description provided.

@anawishnoff anawishnoff requested a review from MikeHillberg May 1, 2020 20:46
@anawishnoff anawishnoff requested a review from a team as a code owner May 1, 2020 20:46
: Windows.UI.Xaml.DependencyObject
{
// vvvv
RowDefinition(double height, GridUnitType type);
Copy link
Contributor

Choose a reason for hiding this comment

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

A few questions:

  1. If there is a difference between "*" and "*1" how are users able to set "*"? Should they set height to 0?
  2. What about GridUnitType.Auto? Will height be ignored in that case? It has to essentially right?
  3. Out of curiosity should Double.NaN be considered an error for GridUnitType.Pixel and GridUnitType.Star or should we do some special logic for that?

Or are this all topics that are just handled by GridLength and we should just pass the arguments and do no checking at all?

Choose a reason for hiding this comment

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

My assumption on behavior based on current behavior:
1: Setting it to 0* would give it 0 height. You should set the number to the fractions you want the height to be. Same as it works in Xaml.
2. We already have the static GridLength.Auto getter (unless you use the WinUI3 alpha where it's oddly missing). If you use the type-overload, the number won't really matter.
3. I'd vote for an error here. Perhaps Auto could allow NaN, but would prefer consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for bringing these things up! I'm not too sure if these are handled by GridLength, but I'll specify these special cases in the spec remarks.

Good catch with GridLength.Auto - I agree that the number should be ignored if you specify Auto as your GridUnitType.

As for NaN, I think an error would be most appropriate for all GridUnitTypes - if someone is calculating a NaN value for their layout, it would seem that there's some kind of larger underlying issue.

Copy link

@dotMorten dotMorten left a comment

Choose a reason for hiding this comment

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

Nice subtle improvement. While probably out of scope for this PR, I'm curious if you also would consider simplifying adding a set of definitions. If I had to add 4 rows, I could merely say something like
myGrid.RowDefinitions = new RowDefinitionCollection(new GridLength(5).new GridLength(5, GridUnitType.Star), GridLength.Auto);
I realize that would also require RowDefinitions to be settable.
An alternative would be to add an AddRange method to it.

var grid = new Grid();
var column = new ColumnDefinition();
column.Width = new GridLength(1.0, GridUnitType.Star);
grid.ColumnDefinitions.Add(column);

Choose a reason for hiding this comment

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

This comparison seems a little unfair. These 4 lines could be written as:

var grid = new Grid();
grid.ColumnDefinitions.Add(new ColumnDefinition() { Width = new GridLength(1.0, GridUnitType.Star) });

That makes it almost identical to the new syntax (just not using the new proposed constructor)

Copy link
Contributor

Choose a reason for hiding this comment

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

If one uses C#, you can write that. In C++ you don't have that C# syntactic sugar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points. It's true that you don't get the same kind of syntactic sugar in C++, but also I was mainly writing the sample out in this way so it makes sense with the steps that I listed above:

  1. Create a Column or RowDefinition object with the blank constructor.
  2. Define a GridLength object with the desired value for the Height or Width property of the Row/ColumnDefinition.
  3. Assign the GridLength object to the Height or Width property of the Row/ColumnDefinition.

@anawishnoff
Copy link
Contributor Author

Nice subtle improvement. While probably out of scope for this PR, I'm curious if you also would consider simplifying adding a set of definitions. If I had to add 4 rows, I could merely say something like
myGrid.RowDefinitions = new RowDefinitionCollection(new GridLength(5).new GridLength(5, GridUnitType.Star), GridLength.Auto);
I realize that would also require RowDefinitions to be settable.
An alternative would be to add an AddRange method to it.

Thanks @dotMorten! I like this idea. I think that changing RowDefinitions to be settable might be too big of a change for the scope of this change (might open up a whole other can of worms), but adding an AddRange method to RowDefinitions seems like a simple and very useful change. It would be a seemingly simple API update to add that method... @MikeHillberg do you think adding an AddRange() method to RowDefinitions and ColumnDefinitions would pose any issues?

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.

3 participants