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 check for undefined header in Column resizer #1156

Merged
merged 9 commits into from
May 10, 2023

Conversation

LostABike
Copy link
Contributor

@LostABike LostABike commented Mar 23, 2023

Changes

When resizing the last column that is column manager in expand mode, the table crashes with error "cannot read properties of undefined" which seem to occur from using getHeaderWidth function. So I added a check within that function to see whether if header is undefined and return 0.

Closes #1004

Testing

  • Tested column manager table in Storybook with extra prop isResizable=true and columnResizerMode='expand'

Docs

  • Added changeset

@LostABike LostABike requested a review from a team as a code owner March 23, 2023 17:34
@LostABike LostABike requested review from a team, gretanausedaite and elephantcatdog and removed request for a team March 23, 2023 17:34
@LostABike LostABike self-assigned this Mar 23, 2023
Copy link
Contributor

@elephantcatdog elephantcatdog left a comment

Choose a reason for hiding this comment

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

So this doesn't show the error anymore, but it doesn't allow me to actually resize the column in the example the user sent in. Does it work for you in the storybook story?

@LostABike
Copy link
Contributor Author

LostABike commented Mar 24, 2023

@elephantcatdog the storybook story works.

So this doesn't show the error anymore, but it doesn't allow me to actually resize the column in the example the user sent in. Does it work for you in the storybook story?

The storybook story works-ish..? I'm not too confident about the expected behavior though:
column resize test

For user's example, I think it works too but because it makes the table width increase which cause the horizontal scroll appear while things look unchanged..?
user example

I was thinking that we disable resizing for the the selection/first column, should we do the same for column manager column? That would keep the table width the same too. But I'm not sure what the expected behavior should be.

@elephantcatdog
Copy link
Contributor

elephantcatdog commented Mar 24, 2023

@LostABike Oooh, I see now. I was trying to make the column manager column bigger, but I actually don't see a reason for that to be bigger IMO. In that case, I tested it and this looks good to me.

Except that there's a test failing, but it looks like it's a CSS visual test on tiles, so that's unrelated. I'm going to merge main in and rerun them to see if that fixes the test.

Update: I ran the tests locally and it passed... so 🤷‍♂️

Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

It fixes the error but now I can make columns smaller than table.
image

@LostABike
Copy link
Contributor Author

It fixes the error but now I can make columns smaller than table. image

I can't seem to reproduce this 😮 I used the user's example to run locally in vite, which I believe was what you did as well. Could you elaborate on how you were able to reproduce that bug?

@gretanausedaite
Copy link
Contributor

Could you elaborate on how you were able to reproduce that bug?

Screen.Recording.2023-04-04.at.15.20.01.mov

I think it's something with how we calculate min width of the column based on table width.

Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

I think we can merge this as a hotfix for crashing. Table changing size is better than app not working.
We just need to add a new issue for resizing bugs.

@LostABike @mayank99 What do you think?

@LostABike
Copy link
Contributor Author

I think we can merge this as a hotfix for crashing. Table changing size is better that app not working. We just need to add a new issue for resizing bugs.

@LostABike @mayank99 What do you think?

Yea I think that would be optimal...I could not figure the following bug out x_x

@mayank99
Copy link
Contributor

Sounds good to me. Lets merge this PR and reopen the issue.

@LostABike LostABike added this pull request to the merge queue May 10, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 10, 2023
@mayank99 mayank99 enabled auto-merge May 10, 2023 16:42
@mayank99 mayank99 added this pull request to the merge queue May 10, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 10, 2023
@mayank99 mayank99 merged commit 70043c3 into main May 10, 2023
@mayank99 mayank99 deleted the annie/column-manager-width branch May 10, 2023 18:23
@imodeljs-admin imodeljs-admin mentioned this pull request May 10, 2023
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.

Table: improve resizing for action column
4 participants