Skip to content
This repository has been archived by the owner on Jan 5, 2023. It is now read-only.

fix(Table): Fixes headless Table as previously it started to throw an error #648

Merged
merged 5 commits into from
May 4, 2022

Conversation

bentleyvk
Copy link
Contributor

@bentleyvk bentleyvk commented May 3, 2022

Closes #647

When user doesn't pass columns with root wrapping header, then error is thrown because it can't find any columns for hasManualSelectionColumn check.
Also fixed an error where it would print to console that isDisabled is not a valid prop in EditableCell.

Checklist

  • Add meaningful unit tests for your component (verify that all lines are covered)
  • Verify that all existing tests pass
  • Add component features demo in Storybook (different stories)
  • Approve test images for new stories
  • Add screenshots of the key elements of the component

@bentleyvk bentleyvk requested a review from a team as a code owner May 3, 2022 09:32
@bentleyvk bentleyvk requested review from a team, veekeys and elephantcatdog and removed request for a team May 3, 2022 09:32
@bentleyvk bentleyvk self-assigned this May 3, 2022
@@ -2233,7 +2233,7 @@ it('should handle table resize only when some columns were resized', () => {
triggerResize = onResize;
return [
jest.fn(),
({ disconnect: jest.fn() } as unknown) as ResizeObserver,
{ disconnect: jest.fn() } as unknown as ResizeObserver,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea why it made random formatter changes.

Copy link
Member

Choose a reason for hiding this comment

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

what have you done with your formatter :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing, I saw similar changes in Jon's PR. Our rules must have changed with monorepo

Copy link
Member

Choose a reason for hiding this comment

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

Changed or removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk, ask Mayank

Copy link
Contributor

Choose a reason for hiding this comment

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

It was because of multiple conflicting versions of prettier. I've created #651 to fix it.

@bentleyvk Can you verify that it fixes the issue in your PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied yarn.lock, ran yarn install and tried to format Table.tsx but nothing happened, I also tried to commit the file but still it remain the same - with changed formatting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, had to refresh VS Code, now it formats. But strange why pre-commit hook didn't format...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I just merge before waiting for prettier fix? Because there is a user that is waiting for my fixes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I fixed and pushed the formatting in d6f33ee. Should be good to merge now.

isDisabled,
...rest
} = props;
isDisabled; // To omit and prevent eslint error.
Copy link
Member

Choose a reason for hiding this comment

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

Cant we omit this prop before it even reaches editable cell? Looks weird now..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You want to ask our users not to pass {...props}? 😀
image

Copy link
Member

Choose a reason for hiding this comment

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

ahh... thats weird..
Cant editable cell be disabled, afterall? :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then it would be just a default cell, doesn't make sense.

Copy link
Member

Choose a reason for hiding this comment

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

Thats fine. Users would not need to switch between cells in their custom implementations.
They just pass all the props and be calm about it.

packages/iTwinUI-react/src/core/Table/Table.tsx Outdated Show resolved Hide resolved
@@ -2233,7 +2233,7 @@ it('should handle table resize only when some columns were resized', () => {
triggerResize = onResize;
return [
jest.fn(),
({ disconnect: jest.fn() } as unknown) as ResizeObserver,
{ disconnect: jest.fn() } as unknown as ResizeObserver,
Copy link
Member

Choose a reason for hiding this comment

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

what have you done with your formatter :D

Copy link
Member

@veekeys veekeys left a comment

Choose a reason for hiding this comment

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

I dont like the 'hack' on isDisabled part, cause its kinda fooling the user now, but hopefully the original issue for headless table is fixed.

@bentleyvk bentleyvk merged commit 87c6b9e into main May 4, 2022
@bentleyvk bentleyvk deleted the vilius/headless-table-fix branch May 4, 2022 13:42
mayank99 added a commit to iTwin/iTwinUI that referenced this pull request Dec 21, 2022
… error (iTwin/iTwinUI-react#648)

Co-authored-by: Mayank <mayank99@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table: isDisabled prop appearing in the DOM Table: when there is no wrapping header, Table throws an error
3 participants