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

Min and Max column width not behaving as expected #1622

Open
mtanco opened this issue Sep 14, 2022 · 14 comments
Open

Min and Max column width not behaving as expected #1622

mtanco opened this issue Sep 14, 2022 · 14 comments
Labels
bug Bug in code good first issue Contributions welcome! ui Related to UI

Comments

@mtanco
Copy link
Contributor

mtanco commented Sep 14, 2022

Wave SDK Version, OS

h2o_wave==0.22.0

Actual behavior

The goal is to programmatically make columns widths automatically the right size to fit in a table without scrolling. Upon trying to do this we found some interesting and maybe unexpected behaviors:

  • The size of min_width==0 is bigger than expected
  • Tables are identical when using max_width regardless of if min_width is set or not, in that min_width seems to be ignored if max_width is set
@app('/')
async def serve(q: Q):

    table_rows = [
        ui.table_row(name='row1', cells=['John', 'Doe', "7042 23rd Ave", "123-456-7890"]),
        ui.table_row(name='row2', cells=['John', 'Doe', "7042 23rd Ave", "123-456-7890"]),
        ui.table_row(name='row3', cells=['John', 'Doe', "7042 23rd Ave", "123-456-7890"]),
    ]
    column_names = ["name", "surname", "address", "phone"]
    columns_to_test = {
        "no_width": [ui.table_column(name=x, label=x) for x in column_names],
        "min_width_0": [ui.table_column(name=x, label=x, min_width="0px") for x in column_names],
        "max_width_100": [ui.table_column(name=x, label=x, max_width="200px") for x in column_names],
        "min_width_0_and_max_width_100": [ui.table_column(name=x, label=x, min_width="0px", max_width="200px") for x in column_names]
    }

    q.page["meta"] = ui.meta_card("", layouts=[ui.layout("xs", width="800px", zones=[ui.zone("default")])])

    for k in columns_to_test.keys():
        q.page[k] = ui.form_card(box='default', items=[
            ui.text_xl(k),
            ui.table(name=k, columns=columns_to_test[k], rows=table_rows)
        ])

    await q.page.save()
@mtanco mtanco added the bug Bug in code label Sep 14, 2022
@mturoci mturoci added the ui Related to UI label Sep 14, 2022
@mtanco
Copy link
Contributor Author

mtanco commented Sep 14, 2022

This is not necessarily a bug, or a feature request - it's more tracking for research into what's going on / how to improve the table column experience or even how to just make docs about table better

@mturoci mturoci added the good first issue Contributions welcome! label Oct 5, 2022
@winkle29
Copy link

Hi @mtanco! can I please work on this issue?

@mturoci
Copy link
Collaborator

mturoci commented Oct 13, 2022

Hi @winkle29 I have already assigned #1645 to you already, try finishing that one first before proceeding to others. Thanks!

@sabrinaxiao
Copy link

Hi @mtanco ! I'm currently a student and new to open source. I would like to take up this issue with my partner @magandren !

@mturoci
Copy link
Collaborator

mturoci commented Nov 7, 2022

Go for it @sabrinaxiao!

@magandren
Copy link

magandren commented Nov 7, 2022

Thank you for this opportunity @mturoci! We have a quick question in regards to completing this task. From our understanding, it seems that we are given 4 days in order to complete the task before it is given up to others that are interested in working on it. We both are in a class where our last assignment is making a contribution to an open source project. For our assignment, we will be writing up a report in regards to the project and task we selected along with our planning process by the end of this week. We also are asked to write a second report at the end of this month on our findings and if we were able to complete our task using software engineering concepts we learned in class. As students who may not have enough time to make a meaningful contribution within a couple of days, we were wondering if we can have more time to work on this task. Thank you so much for your time and flexibility!

@mturoci
Copy link
Collaborator

mturoci commented Nov 8, 2022

we are given 4 days in order to complete the task

This was just for the Hacktoberfest that is already passed. No more time limits from now on so feel free to take as much time as necessary :)

@magandren
Copy link

@mturoci Hello! We have some initial clarifying questions in regards to our expected behavior to ensure we going in the right direction!! After running the code, we get the following tables:

Screen Shot 2022-11-16 at 8 32 46 PM

  1. “The size of min_width==0 is bigger than expected” — We are able to see how adding a min_width without a max_width affect the default columns width. As it was bigger than expected, just to clarify, were the table columns of “min_width_0” expected to next to each other with little to no space in between?

  2. “Tables are identical when using max_width regardless of if min_width is set or not, in that min_width seems to be ignored if max_width is set“ — From our understanding, our goal is to have the table columns automatically resize such that we do not have to scroll across. Min_width and Max_width would ensure a limit as to the size of the table columns when the table is adjusted. Is the expected behavior of “max_width_100” table supposed to look like “no_width” table and “min_width_0 _and_max_width_100” table look like “min_width_0”? If this is not the expected behavior, we would appreciate clarification of how “min_width_0 _and_max_width_100” should differ from “max_width_100”. We would be grateful you are able to give some guidance on this issue.

Once again, thank you so much for your help!

@mturoci
Copy link
Collaborator

mturoci commented Nov 18, 2022

As it was bigger than expected, just to clarify, were the table columns of “min_width_0” expected to next to each other with little to no space in between?

You can propose what you think would be ideal. IMO if min_width=0, the col width should a default width (>0 ofc), but I am open to other suggestions as well.

From our understanding, our goal is to have the table columns automatically resize such that we do not have to scroll across

I don't think it's possible to compute the "ideal" width, e.g. the width of the longest string in a column because the table can be paginated, have wrapping set etc. + it would be very slow.

we would appreciate clarification of how “min_width_0 _and_max_width_100” should differ from “max_width_100”

Again, proposals are welcome. IMO min/max should be primarily used for column resizing via mouse and:

  • If min is set and it's > default, then use min
  • if max is set and it's < default, then use max
  • if min < default < max, use default.

@sabrinaxiao
Copy link

@mturoci We have adjusted our code locally such that min_width = 0 would appear the same as a table without a min_width specified. Based on our findings, our default value for min_width is 150px and our default value for max_width is undefined. We noticed that the current behavior of the code matches these two cases you specified:

If min is set and it's > default, then use min
If max is set and it's < default, then use max

Just to clarify, is the expected behavior of: “if min < default < max, use default.” result in both min_width and max_width to be equal to 150px?

Also regarding testing our code changes, where would you like to include the test cases to ensure our results are correct? Is there a format or a resource to follow as to how these test cases should look like?

@mturoci
Copy link
Collaborator

mturoci commented Nov 28, 2022

result in both min_width and max_width to be equal to 150px?

Not sure what you mean exactly. It means that if min=100, default=150 and max=200, the column should be 150px initially and allow resizing up to 100/200 (min/max).

where would you like to include the test cases to ensure our results are correct?

Plain PR screenshots with minimal python example code should be a good start. Ideally, you might want to include the tests within our table unit tests (table.test.tsx) as well, depending on your JS testing knowledge - optional.

@magandren
Copy link

magandren commented Nov 28, 2022

result in both min_width and max_width to be equal to 150px?

Not sure what you mean exactly. It means that if min=100, default=150 and max=200, the column should be 150px initially and allow resizing up to 100/200 (min/max).

I see our confusion my apologies! We primarily looked into table.tsx and found how to manipulate the values of min_width and max_width. We currently are having trouble understanding how the widths of the table columns are set. Do you have any suggestions/guidance on where to look? Thank you so much again.

@mturoci
Copy link
Collaborator

mturoci commented Nov 28, 2022

When approaching a problem in general, it's good to start by looking at how current implementation works, understand it and then do the necessary changes. You can trace the min/max props and see where and how they are used, then think of how to alter the behavior according to your desires. Wave uses Fluent component library under the hood, so you might look there as well.

@magandren
Copy link

magandren commented Dec 3, 2022

@mturoci Thank you so much for this advice! We looked into the Fluent component library and found that the behavior of why “min_width = 0 is bigger than expected” (it’s being set to 100px by default) lies in the code logic of FluentUI here .

In our findings, we looked into the behaviors of min_width and max_width. We found that if min_width = 0 or undefined and max_width was set, according to the logic of FluentUI, the min_width will be set to the max_width. This explains the unexpected behavior in which “min_width seems to be ignored if max_width is set”.

In addition to the behaviors we were assigned to investigate, we also found an additional unexpected behavior. In FluentUI, if the max_width is set, the variable is not utilized at all. As a result, despite the max_width being set, the user is still able to stretch the table column width indefinitely with no restriction. As an example, in one of the unit tests in FluentUI, you can see here that the max_width = 400px however this unit test tries to set the max_width to 500px. Potentially in the future, this feature will be fully implemented in FluentUI.

As most of these unexpected behaviors are due to FluentUI, as a temporary solution for the “min_width = 0 is bigger than expected” behavior, we decided to change min_width such that if the user would like to set min_width = 0px, it will be displayed as a non zero value (0.001px) which matches our expected behavior. Making this quick fix also resolved the unexpected behavior of “Tables are identical when using max_width regardless of if min_width is set or not, in that min_width seems to be ignored if max_width is set”. The min_width is no longer ignored with this fix. We have submitted a pull request of our changes to reflect this min_width change.

This concludes our investigation! Any feedback is greatly appreciated!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug in code good first issue Contributions welcome! ui Related to UI
Projects
None yet
Development

No branches or pull requests

5 participants