Skip to content

Conversation

@jenny-s51
Copy link
Contributor

What: Closes #9030

@jenny-s51 jenny-s51 requested review from nicolethoen and tlabaj May 4, 2023 18:39
@patternfly-build
Copy link
Contributor

patternfly-build commented May 4, 2023

@jenny-s51 jenny-s51 changed the title chore(table demos): update filterable demo from legacy to new table chore(table demos): update filterable demo from deprecated to new table May 4, 2023
Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

I see a few things we could clean up while we are tinkering with this demo:

Looks like you can remove the following unused dependencies:

Dropdown,
DropdownItem,
DropdownPosition,
DropdownToggle,

minWidth is being passed twice to the Select on line 1376
filters is not ever used - defined on line 1399

Copy link
Contributor Author

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

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

Thanks @nicolethoen ! updated

Copy link
Contributor

@nicolethoen nicolethoen 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 MenuToggle and MenuToggleElement need to be imported by your demo.
(BTW, i catch these by trying to open full page demos in a codesandbox when i review them. There are errors right now preventing them from working in a codesandbox - unrelated to your changes - but it still catches these sorts of unused vars and missing imports problems)

Copy link
Contributor Author

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

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

Thank you @nicolethoen - added the missing imports. I'll be double-checking any demo PRs in codesandbox from now on 🙂

Good find on the import path - assigned myself to patternfly/patternfly-org#3525, will be looking into it

@nicolethoen
Copy link
Contributor

Good find on the import path - assigned myself to patternfly/patternfly-org#3525, will be looking into it

I think @evwilkin has done work on updating relative imports in codesandbox imports in the past. So once he's back, he may have good insights here

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

lgtm

@nicolethoen nicolethoen merged commit d7cd80a into patternfly:v5 May 11, 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 - Filterable demo cleanup

4 participants