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 table/grid column width in fixed, sizeToConent (auto) and star (proportional) #707

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

Conversation

BobSilent
Copy link

@BobSilent BobSilent commented Jan 26, 2022

This PR allows to set the column witdh not only to fixed width or auto-size (size to content)
but also allows proportional sizing:

table.AddColumn(new TableColumn("Column 1").StarWidth(1.5));
table.AddColumn(new TableColumn("Column 2").StarWidth(1));

The numbers do not have to be integers.
In this example, column 1 is 1.5 times wider than column 2.

Also available:

table.AddColumn(new TableColumn("Column 1").FixWidth(10));
table.AddColumn(new TableColumn("Column 2").FixWidth(10));
table.AddColumn(new TableColumn("Column 3").SizeToContent());

You can also mix auto-fit and fixed widths with star (proportional) widths; in that case the star columns are apportioned to the remainder after the auto-fit and fixed widths have been calculated:

table.AddColumn(new TableColumn("Column 1").FixWidth(10));
table.AddColumn(new TableColumn("Column 2").StarWidth(1));
table.AddColumn(new TableColumn("Column 3").StarWidth(2));
table.AddColumn(new TableColumn("Column 4").SizeToContent());

This is one part for #518

unfortunately there is a breaking change in new TableColumn("").Width(10) to new TableColumn("").FixWidth(10)


Please upvote 👍 this pull request if you are interested in it.

@patriksvensson patriksvensson self-requested a review January 27, 2022 13:01
Copy link
Contributor

@patriksvensson patriksvensson left a comment

Choose a reason for hiding this comment

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

I can't see anything wrong with the code except the breaking change, but I need tests that demonstrate the added functionality.

@BobSilent
Copy link
Author

Shall I extend the examples\Console\Tables\Tables.csproj with an example?

@jouniheikniemi
Copy link
Contributor

A lurker commenting here :-) Am I the only person who thinks "Star" is a fairly obscure way to refer to proportional scaling (unless you have a CSS/WPF/... background that is)? "ProportionalWidth" maybe?

@BobSilent BobSilent force-pushed the add-table-grid-star-sizing branch from 4732d14 to d5a32d5 Compare January 28, 2022 14:25
@BobSilent
Copy link
Author

I am open for better naming's, ProportionalWidth is quite lengthy and is it more precise or clear?
That's why I took the Star name, which is (I thought) quite common in dotnet and CSS.

@phil-scott-78
Copy link
Contributor

The Star name wasn't something I intuitively understood either. ProportionalWidth would also be my preference.

@patriksvensson
Copy link
Contributor

I was thinking a bit. Perhaps we could add overloads to the Width method, which takes a ProportionalWidth struct. That way it would be easily discoverable for users.

table.AddColumn(new TableColumn("Column 1").Width(10));
table.AddColumn(new TableColumn("Column 2").Width(new ProportionalWidth(1.5)));

@phil-scott-78 @nils-a What do you think?

@phil-scott-78
Copy link
Contributor

I like that. Might make making it easier to expand this idea to things like a progress bar too

@nils-a
Copy link
Contributor

nils-a commented Jan 28, 2022

Yes, sounds good. Maybe even add a little extension to write it like:

table.AddColumn(new TableColumn("Column 2").Width(1.5.Proportions()));

@BobSilent
Copy link
Author

okay, i will prepare a proposal.

What about the IColumn.Width property, would it make sense to also change it, to not be double? but of Type ColumnWidth like that

public class ColumnWidth
{
    public SizeMode SizeMode
    {
        get;
    }

    public double Value
    {
        get;
    }

    private ColumnWidth(SizeMode sizeMode, double value)
    {
        SizeMode = sizeMode;
        Value = value;
    }

    public static ColumnWidth Fixed(int size)
    {
        if (size < 0.0)
        {
            throw new ArgumentException("Fixed size cannot be negative", nameof(size));
        }

        return new ColumnWidth(SizeMode.Fixed, size);
    }

    public static ColumnWidth Proportional(double weight = 1.0)
    {
        if (weight < 0.0)
        {
            throw new ArgumentException("Weight cannot be negative", nameof(weight));
        }

        return new ColumnWidth(SizeMode.Star, weight);
    }

    public static ColumnWidth SizeToContent()
    {
        return new ColumnWidth(SizeMode.SizeToContent, 0.0);
    }
}

than also the Syntax could be aligned

new TableColumn("Column").ProportionalWidth(1.5);

new TableColumn("Column")
{
    Width = ColumnWidth.Proportional(1.5)
}

@nils-a
Copy link
Contributor

nils-a commented Jan 31, 2022

Sounds good. Should we add an implicit conversion from double to ColumnWidth, so we don't break every single use of the existing Width?

@khellang
Copy link
Contributor

It seems unnecessary to break the IColumn interface just to convert the Width from int? to double?. What's the benefit?

@khellang
Copy link
Contributor

Also, if you want to avoid breaking changes here, I think it should be considered having a default implementation of SizeMode (returning today's default)

@BobSilent
Copy link
Author

BobSilent commented Jan 31, 2022

Sounds good. Should we add an implicit conversion from double to ColumnWidth, so we don't break every single use of the existing Width?

I updated the PR with the comments. A remarkable part is maybe this line
1739e1d#diff-4280ceab8b9bb14f6f7460e6651481d450117645c33d18e00ec05d92647d6b91R82-R86

I reverted the extensions back to have only a single Width and did not add yet a ProportionalWidth
as it could be now written like

new TableColumn("Column").Width(ColumnWidth.Proportional(1.5));
new TableColumn("Column").Width(ColumnWidth.SizeToContent());
new TableColumn("Column").Width(ColumnWidth.Fix(3));

does it still make sense to have the additional ProportionalWidth and SizeToContent overloads?
ColumnWidth has an implicit conversion from int? (which is the original implementation) to ColumnWidth.
which preseves the existing defaults:

  • ColumnWidth.Fix(value) if value is not null
  • ColumnWidth.SizeToContent() if given value is null.

@BobSilent
Copy link
Author

@patriksvensson did I miss something? Or how can I support to get this merged?

@patriksvensson
Copy link
Contributor

@BobSilent Sorry for taking so long. Life has been hectic. I will set aside some time tomorrow and look through this.
Sorry again.

@BobSilent
Copy link
Author

@patriksvensson No problem, take the time you need!
I just wasn't sure: In the beginning the feedback came pretty quickly (and a lot) and suddenly it went quiet. So it wasn't quite clear to me how to understand that - maybe an "undesirable" PR 😃
So the short feedback is enough for me - at the moment 😉

If there are any wishes or suggestions for improvement, please let me know, I would be happy to make my contribution.

@patriksvensson
Copy link
Contributor

@BobSilent Absolutely not undesirable! Very happy with what you've done.

@BobSilent
Copy link
Author

@patriksvensson any updates here? 😉

Copy link
Contributor

@patriksvensson patriksvensson left a comment

Choose a reason for hiding this comment

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

Initially, I think it looks good.
We would need tests that cover the new measuring functionality, though.

@BobSilent BobSilent force-pushed the add-table-grid-star-sizing branch from 1739e1d to 77afd34 Compare August 1, 2022 08:28
@BobSilent
Copy link
Author

@patriksvensson I updated the docu

@patriksvensson
Copy link
Contributor

patriksvensson commented Aug 29, 2022

@BobSilent

@patriksvensson I updated the docu

That's great! However, we need tests that cover the new functionality to be able to merge this.

@BobSilent
Copy link
Author

Ok, I see. Can you give me a hint for the mentioned functionality and other similar tests.
Then I will provide some tests. This week or by end of next week latest.

@patriksvensson
Copy link
Contributor

I will get back to you with some examples as soon as I have some spare time.

@BobSilent
Copy link
Author

@patriksvensson can you please provide me some examples, as I really would like to get this merged 😉

@BobSilent
Copy link
Author

@patriksvensson I think you forgot me...

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.

6 participants