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

Proposal: Simplify Grid Column and Row Definitions #673

Closed
dotMorten opened this issue May 9, 2019 · 133 comments
Closed

Proposal: Simplify Grid Column and Row Definitions #673

dotMorten opened this issue May 9, 2019 · 133 comments
Assignees
Labels
area-Layouts area-Tooling feature proposal New feature proposal needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) team-Markup Issue for the Markup team

Comments

@dotMorten
Copy link
Contributor

dotMorten commented May 9, 2019

Summary

The proposal is to simplify the definition syntax of Rows and Columns by:

  • Allowing rows and columns within a Grid to be defined by a collection that is delimited by commas and single quotes.
  • Assigning the content property of ColumnDefinition to Width and RowDefinition to Height, and defining the CreateFromString attribute for ColumnDefinitions and RowDefinitions classes, which will allow the shortened syntax to work.

Goal

The goal of this feature is to make Grid easier and more intuitive to write and learn. A major consideration for this proposal is to assure that any new syntax can work as a generalized language feature to fit other use cases across XAML.

Example

Current Syntax

Defining columns and rows is overly tedious, repetitive and not very productive, as is shown below:

<Grid>
    <Grid.ColumnDefinitions>
          <ColumnDefinition Width="1*" />
          <ColumnDefinition Width="2*" />
          <ColumnDefinition Width="Auto" />
          <ColumnDefinition Width="*" />
          <ColumnDefinition Width="300" />
    </Grid.ColumnDefinitions>
    <Grid.RowDefinitions>
          <RowDefinition Height="1*" />
          <RowDefinition Height="Auto" />
          <RowDefinition Height="25" />
          <RowDefinition Height= "14" />
          <RowDefinition Height="20" />
    </Grid.RowDefinitions>
</Grid>

Proposed Syntax

The same functionality as above with the proposed succinct syntax is shown below:

<Grid ColumnDefinitions="1*, 2*, Auto, *, 300"
      RowDefinitions="1*, Auto, 25, 14, 20">
</Grid>

The following syntax will also be valid after the new language feature is implemented:

<Grid>
    <Grid.ColumnDefinitions>
          <ColumnDefinition>1*</ColumnDefinition>
          <ColumnDefinition>2*</ColumnDefinition>
          <ColumnDefinition>Auto</ColumnDefinition>
          <ColumnDefinition>*</ColumnDefinition>
          <ColumnDefinition>300</ColumnDefinition>
    </Grid.ColumnDefinitions>
    <Grid.RowDefinitions>
          <RowDefinition>1*</RowDefinition>
          <RowDefinition>Auto</RowDefinition>
          <RowDefinition>25</RowDefinition>
          <RowDefinition>14</RowDefinition>
          <RowDefinition>20</RowDefinition>
    </Grid.RowDefinitions>
</Grid>

Rationale

A new grid syntax would not only improve developer satisfaction and productivity but would also minimize the learning curve required to use Grid, one of the most widely used controls. The original syntax would be kept in place to use for more complex scenarios and so older applications don’t have to be changed.

Implementing the succinct syntax will require the creation of a new XAML language feature, as typeconverters cannot be created for or used on read-only collections (which ColumnDefinitions and RowDefinitions are). In creating this new language feature, we wanted to make sure the syntax can be generalized for possible future use cases, including collections that may include Point objects, sets, or even string values that contain commas themselves. A comma-and-quotes-separated list is intuitive for single values (such as column heights), but is also readable for more complex types, such as Points. Examples of different syntax scenarios are below:

Succinct syntax for Grid definitions (numerical values)

<Grid ColumnDefinitions="1*, 2*, Auto, *, 300"/>

Succinct syntax for more complex types of values

// A list of points: 
<Foo PointList=" '1,2', '9,4', '2,6'"/>

// A collection of strings:
<Bar Words=" 'hello', 'world', 'hello, world!', 'it&apos;s a beautiful day'" /> 
//note that apostrophe/single quote character can be escaped by using the HTML entity.

// Collection of dates for CalendarView:
<CalendarView SelectedDates="'1/5/1977', '6/19/1997'" />

Scope

Feature Priority
Ability for rows and columns to be defined as a collection or list of values (using CreateFromString attribute) Must
Content properties of RowDefinition and ColumnDefinition are set to Height and Width, respectively Must
Original syntax will still be fully supported and functional Must
Uses a syntax that can eventually be generalized across all XAML read-only collection-typed properties Must
Include support for min/max height/width Won't
Include support for data binding within definitions Won't
Include support for blank values or default values Won't

Appendix

A syntax that uses a list separated by commas and quotes was chosen due to:

  • An overwhelming response from the community in favor of a comma delimiter
  • The ability of single quotes to separate more complex structures such as Point objects and sets as well as support strings that use commas
  • The combination of the two makes it so collections containing only simple types will require only commas as a delimiter, and collections containing more complex types will use single quotes and commas as delimiters, keeping everything intuitive and using familiar syntax patterns for developers.
@dotMorten dotMorten added the feature proposal New feature proposal label May 9, 2019
@mdtauk
Copy link
Contributor

mdtauk commented May 9, 2019

Might I suggest there be consideration for the ability to add a name or key to columns, do you can declare it in XAML.

<Button Grid.Row="Toolbar" />

@dotMorten
Copy link
Contributor Author

@mdtauk that's part of the bigger issue I link to. This issue was specifically for a small subset that doesn't add any new feature but just simplifies the xaml

@mdtauk
Copy link
Contributor

mdtauk commented May 9, 2019

You mention string conversion, so I thought adding a string value for a name could translate to an int value assigned Inthe definition lists?

@dotMorten
Copy link
Contributor Author

But named columns is adding an entirely new feature. I purposely tried to keep this suggestion without adding new features.

@michael-hawker
Copy link
Collaborator

Yeah, it'd be great if we could add this exact same shortcut to both UWP Grid and WPF Grid.

@MartinZikmund
Copy link
Contributor

Great idea, love it!

@Sergio0694
Copy link
Member

Sergio0694 commented May 9, 2019

This looks great!
Also, I like the fact that we can actually already achieve this with attached properties, while we wait for this to be officially implemented into the various frameworks 😄

I have a small issue with that syntax though, I feel like the ,,, syntax for default rows/columns might be a bit error prone and possibly difficult to read while glancing over the code. 🤔
Also, the fact that having just 3 commas would actually correspond to 4 rows/columns.
What about using *,*,*,* (or 1*,1*,1*,1* if you will), so that it's easier to read and less likely to get wrong?

@bmacombe
Copy link

bmacombe commented May 9, 2019

@dotMorten This would probably be a good addition to Xamarin Forms if it hasn't been proposed already. Maybe after it's discussed well here first.

@andrund
Copy link

andrund commented May 9, 2019

This is great and exactly what som people have done in custom controls, so it feels familiar. Any ideas how to handle Max/Min Width/Height.

@dotMorten
Copy link
Contributor Author

Any ideas how to handle Max/Min Width/Height.

Ah shucks. Didn't think about that. One simple solution is just to state you don't get that fine grained control with the simple syntax (in order to keep the syntax simple and focus on covering the most common scenarios).

@mdtauk
Copy link
Contributor

mdtauk commented May 9, 2019

We should avoid introducing a simpler syntax that would eventually be replaced by the more robust and powerful new syntax and functionality.

So whatever is decided about this simple width/height syntax, it needs to keep in mind what comes next

@mrlacey
Copy link
Contributor

mrlacey commented May 9, 2019

What about using ,,, (or 1*,1*,1*,1* if you will), so that it's easier to read and less likely to get wrong?

This is inconsistent with other shortcuts (such as margin, or padding) where commas (or spaces) are between values, not after them.
I would find the trailing comma confusing.

@Sergio0694
Copy link
Member

Sergio0694 commented May 9, 2019

@mrlacey wait, I think you misquoted my message 🤔
I 100% agree that having a trailing comma wouldn't be ok, that's exactly why I posted that suggestion. The trailing commas are actually in the first post of this proposal, not in my message.

What I suggested (just as an idea, not necessary the best possible syntax) was to use *,*,*,* or 1*,1*,1*,1* instead of ,,, like the first post suggested, for precisely the reason you just mentioned.

EDIT: I mean, in the first post of this proposal you can actually read RowDefinitions="1*,Auto,25,,", which has indeed the issue of inconsistent syntax, with trailing commas and commas with no values in between.

@mrlacey
Copy link
Contributor

mrlacey commented May 9, 2019

@Sergio0694 my mistake. you are correct.
@dotMorten what did you mean by the syntax RowDefinitions="1*,Auto,25,,"?

@mdtauk
Copy link
Contributor

mdtauk commented May 9, 2019

I don't think you should be able to add empty rows, so each entry would need at least a *, so having a collection of commas without values would be invalid

@Sergio0694
Copy link
Member

@mdtauk Precisely what I was suggesting 😄
But that might very well just have been a typo in the first post.

@RichiCoder1
Copy link

Some inspiration from CSS Grid would be great here (though possibly throw the scope out)

RowDefinitions="20, 1*, Minmax(30, 2*), Repeat(3, 200)" and so on.

@msft-github-bot msft-github-bot added this to Freezer in Feature tracking May 10, 2019
@dotMorten
Copy link
Contributor Author

@mrlacey

what did you mean by the syntax RowDefinitions="1*,Auto,25,,"?

The equivalent XAML was just above it

@thomasclaudiushuber
Copy link
Contributor

thomasclaudiushuber commented May 10, 2019

@dotMorten First of all, I really like this issue, as it's a quick win! Great idea to extract this from the bigger issue.

The equivalent XAML was just above it

Yes, but the equivalent XAML for ,, are two 1* RowDefinitions. So, I think - like @mrlacey and @Sergio0694 mentioned - we shouldn't have empty values in the new short-hand syntax, as you should be able to use that syntax also without commas, but spaces instead (like you can for example for Margin).

These should be possible options:

<Grid RowDefinitions="1*,Auto,25,1*,1*"/>
<Grid RowDefinitions="*,Auto,25,*,*"/>
<Grid RowDefinitions="1* Auto 25 1* 1*"/>
<Grid RowDefinitions="* Auto 25 * *"/>

But not empty values like here, as it doesn't allow the space-instead-of-comma-syntax:

<Grid RowDefinitions=",Auto,25,,"/>

@corradocavalli
Copy link

I Love it! 👍

@jsuarezruiz
Copy link
Member

Very nice idea!

@MichaelRumpler
Copy link

Good idea!

The same could be done for cell positions. Instead of

<TextBlock Grid.Column="2" Grid.ColumnSpan="3" Grid.Row="5" Grid.RowSpan="2" Text="Something" />

one could write

<TextBlock Grid.Cell="2-4/5-6" Text="Something" />

(I assume that the ranges are inclusive here, but I may be overruled as I was with System.Range)

This would also not be a new feature, but only a simpler syntax.

I do realize that / and - did not appear in XAML string properties yet, but IMHO this is more readable than just some numbers and ,. I always have the problem with Margin that I don't know which value is top, right, bottom and left as this is different in XAML and CSS. But / and - cannot easily be misinterpreted.

@dotMorten
Copy link
Contributor Author

@MichaelRumpler interesting idea but it again goes against the premise of this issue by introducing a new API (Grid.Set/GetCell and not sure what the type would be?) and this introducing more red tape to get it through. Issue #51 is where the more advanced stuff is discussed.

@wbokkers
Copy link

I like it! It would be nice to still support databinding with this syntax:

<Grid RowDefinitions="{x:Bind Vm.FirstRowDefinition},1*,Auto">

@dotMorten
Copy link
Contributor Author

@wbokkers I don't think that's possible with a string converter. I think the idea of keeping it simple for the 80% use case, and use full syntax when you need more fine-grained control is the best/simplest/most-achievable way to go.

Trying to solve all possible suggestions will just lead to a complicated design with a million devilled details that we'd never be able to push through.

@Sergio0694
Copy link
Member

Also @MichaelRumpler - you probably already know this, but just in case, if you really want to use that Grid.Cell property (or equivalent), you can just implement your own attached property that takes a string with that syntax you used, parses it and then sets all the various Grid.Row, Grid.RowSpan, Grid.Column and Grid.ColumnSpan attached properties on the target control 👍

@thomasclaudiushuber
Copy link
Contributor

thomasclaudiushuber commented May 10, 2019

Yes, I 100% agree with @dotMorten and @Sergio0694.

@MichaelRumpler I like that syntax, but it would mean we have to add another attached property that gives you exactly the same behavior that we have already with the existing properties. So, it's pretty much a "proxy property". I think what @Sergio0694 said is good, using an attached property that doesn't necessarily have to be in the Grid class works for this.

@wbokkers I think that's a really hard one. x:Bind generates code at compile-time, the string is converted at runtime. In addition, the markup extension is only half of the attribute value. I've never seen something like that. While it could make sense, I think it's a very complicated thing to implement. And I can't remember many cases where I needed a data binding for the sizes of RowDefs/ColDefs, so I'd say, let's keep it simple, and if a dev needs a data binding, they could still go with the normal object element syntax.

So, let's keep this issue simple, so that we can get it through. I think this was also the idea why @dotMorten created this separate issue, because it's a quick win that is not too hard to implement.

If I see it correctly, @Sergio0694, @mrlacey, @mdtauk, and I agreed that we don't want empty values between the commas, and no one else said empty values should be supported (Except the initial proposal from @dotMorten ).

So, @dotMorten, the rest of this comment is for you. :-)

What do you think with adjusting the initial proposal with the possible syntax options I wrote in my previous comment? And getting rid of this:

An empty/no value is equivalent to not setting Width/Height and the default is used.

And add this:

Should be a comma-separated or space-separated list.

And add here additional, and write that you might want to do this for better readability. Ignoring the spaces doesn't give you a better readability, you add them to get a better readability, and the string converter ignores them (The additional spaces, as you should be able to split the values also by spaces):

Additional spaces are allowed/ignored (you might want to add them for better readability).

And maybe write explicitly that min/max values are not supported. Then we can have another issue once this is implemented.

@wbokkers
Copy link

You're right in not wanting to overcomplicate things. I use databinding here in maybe 0.01% of the grids I use.

@StephenLPeters StephenLPeters removed the needs-triage Issue needs to be triaged by the area owners label Feb 19, 2021
@wbokkers
Copy link

This works great with WinUI 3 Desktop. But unfortunately it breaks XAML Hot Reload.

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Sep 22, 2021
@MikeHillberg
Copy link
Contributor

@RealTommyKlein

@ghost ghost removed the needs-triage Issue needs to be triaged by the area owners label Sep 22, 2021
@AlexanderBlackman
Copy link

AlexanderBlackman commented Jan 21, 2022

This works great with WinUI 3 Desktop. But unfortunately it breaks XAML Hot Reload.

I can confirm this is still a problem in Visual Studio 22 (17.0.4)

It does not affect the app itself, but still gives an error and stops hot reload.
Error XHR0013 The property "RowDefinitions" does not have an accessible setter

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Jan 21, 2022
@StephenLPeters
Copy link
Contributor

@evelynwu-msft @RealTommyKlein FYI

@ghost ghost removed the needs-triage Issue needs to be triaged by the area owners label Feb 10, 2022
@StephenLPeters StephenLPeters added area-Tooling team-Markup Issue for the Markup team and removed team-Controls Issue for the Controls team labels Feb 10, 2022
@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Feb 10, 2022
@RealTommyKlein
Copy link
Contributor

Should be fixable by updating the tooling on WASDK's end to understand how to instantiate collections from strings the same way this feature did for markup loaded normally. But that is more of a feature-level request which wouldn't meet the bar for Windows App SDK 1.1.

@ghost ghost removed the needs-triage Issue needs to be triaged by the area owners label Feb 10, 2022
@lukasf
Copy link

lukasf commented Nov 9, 2022

This works great with WinUI 3 Desktop. But unfortunately it breaks XAML Hot Reload.

I can confirm this is still a problem in Visual Studio 22 (17.0.4)

It does not affect the app itself, but still gives an error and stops hot reload. Error XHR0013 The property "RowDefinitions" does not have an accessible setter

Is this being worked on? It's not only that changes to grid definitions do not work, but hot reload is completely broken, if you are using the simplified syntax on a page. That's not acceptable.

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Nov 9, 2022
@Balkoth
Copy link

Balkoth commented Mar 2, 2023

Can hot reload please be fixed?

@dotMorten
Copy link
Contributor Author

You should log a visual studio bug using the feedback tool. I agree this is super annoying and the main reason I don't even use the feature I suggested.

@evelynwu-msft
Copy link
Contributor

@Balkoth We've been tracking that issue internally and haven't forgotten about it, but unfortunately it's not likely to be fixed soon (I'm sorry, I'm sure that wasn't the outcome you were hoping for). Feel free to create a separate issue in this repo, though, if you'd like to be notified when the internal status changes.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the needs-triage Issue needs to be triaged by the area owners label Mar 2, 2023
@Balkoth
Copy link

Balkoth commented Mar 2, 2023

To me its crazy that hot reload always gets mentioned when the question about a real designer comes up and then hot reload gets broken by new features. I will just ignore this feature, as it is not worth the hassle.

@AMArostegui
Copy link

AMArostegui commented Mar 16, 2023

To me its crazy that hot reload always gets mentioned when the question about a real designer comes up and then hot reload gets broken by new features. I will just ignore this feature, as it is not worth the hassle.

This is on point. If Hot Reload is meant to be the "alternative" to XAML Designer, at least the team should keep focus on robustness. Not being able to dynamically change cols/rows would be acceptable, but the syntax breaks the feature completely

@MartinZikmund
Copy link
Contributor

I have hit the hot reload issue as well, indeed after the removal of all RowDefinitions and ColumnDefinitions attributes hot reload works reliably.

@AMArostegui
Copy link

AMArostegui commented Mar 16, 2023

The problem is for the unsuspecting developer using the feature. After some work he discovers Hot Reload is not working, and needs to traceback until the moment it did. Just then he realizes that the abbreviated syntax provokes this bug. This is time consuming and annoying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Layouts area-Tooling feature proposal New feature proposal needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) team-Markup Issue for the Markup team
Projects
Feature tracking
Code Complete, needs Docs/Samples
Development

No branches or pull requests