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

🐾 Update to PF5 - part II - handle rest of event handlers #1287

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

sgratch
Copy link
Collaborator

@sgratch sgratch commented Jul 17, 2024

Reference: #1098

For avoiding uncaught PF 4 -> PF 5 migration errors, this PR named and typed (i.e, add type to function signature) the following callbacks appearances which use parameters:
onFilter
onTextInput
onClear
onDrop
onInputKeyDown
onSetPage
onPerPageSelect
onUserMinus
onUserPlus
onDataChange
onTextChange

@sgratch sgratch requested a review from yaacov July 17, 2024 14:44
@@ -57,7 +57,10 @@ export const AutocompleteFilter = ({

const options = validSupported.map(({ label }) => <SelectOption key={label} value={label} />);

const onFilter = (_, textInput) => {
const onFilter: (
e: React.ChangeEvent<HTMLInputElement> | null,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
e: React.ChangeEvent<HTMLInputElement> | null,
event: React.ChangeEvent<HTMLInputElement> | null,

@@ -350,6 +350,27 @@ export function StandardPage<T>({
.filter((field) => field.filter?.primary)
.map(toFieldFilter(sortedData));

const onSetPage: (
_evt: React.MouseEvent | React.KeyboardEvent | MouseEvent,
Copy link
Member

@yaacov yaacov Jul 17, 2024

Choose a reason for hiding this comment

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

Suggested change
_evt: React.MouseEvent | React.KeyboardEvent | MouseEvent,
_event: React.MouseEvent | React.KeyboardEvent | MouseEvent,

@yaacov yaacov added the enhancement Categorizes issue or PR as related to a new feature. label Jul 17, 2024
@yaacov yaacov added this to the 2.6.4 milestone Jul 17, 2024
@sgratch sgratch force-pushed the handle_rest_of_event_handler_for_pf5 branch from 219d5ff to 24c1c1f Compare July 18, 2024 11:33
@sgratch sgratch requested a review from yaacov July 18, 2024 11:41
@@ -87,7 +87,10 @@ export const GroupedEnumFilter = ({
</SelectGroup>
));

const onFilter = (_, textInput) => {
const onFilter: (
e: React.ChangeEvent<HTMLInputElement> | null,
Copy link
Member

Choose a reason for hiding this comment

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

event

};

const onPerPageSelect: (
_evt: React.MouseEvent | React.KeyboardEvent | MouseEvent,
Copy link
Member

Choose a reason for hiding this comment

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

event

@sgratch sgratch force-pushed the handle_rest_of_event_handler_for_pf5 branch from 24c1c1f to c2c5baa Compare July 18, 2024 12:34
@sgratch sgratch requested a review from yaacov July 18, 2024 12:51
@@ -350,6 +350,27 @@ export function StandardPage<T>({
.filter((field) => field.filter?.primary)
.map(toFieldFilter(sortedData));

const onSetPage: (
_event: React.MouseEvent | React.KeyboardEvent | MouseEvent,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_event: React.MouseEvent | React.KeyboardEvent | MouseEvent,
event: React.MouseEvent | React.KeyboardEvent | MouseEvent,

no need for _ in the type definition

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct, but this is how it was declared in PF for the function so I wanted to keep the same signature as much as possible.
https://github.com/patternfly/patternfly-react/blob/db0cdde6e3e6ae61348129b73bb43c97a98b4617/packages/react-core/src/components/Pagination/Pagination.tsx#L82

Anyway, I can see now that it was fixed for PF react 5, so will fix it in our code as well.

};

const onPerPageSelect: (
_event: React.MouseEvent | React.KeyboardEvent | MouseEvent,
Copy link
Member

Choose a reason for hiding this comment

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

Reference: kubev2v#1098

For avoiding uncaught PF 4 -> PF 5 migration errors, this PR named and typed
(i.e. (add type to function signature) the following callbacks appearances which use parameters:
 onFilter
 onTextInput
 onClear
 onDrop
 onInputKeyDown
 onSetPage
 onPerPageSelect
 onUserMinus
 onUserPlus
 onDataChange
 onTextChange

Signed-off-by: Sharon Gratch <[email protected]>
@sgratch sgratch force-pushed the handle_rest_of_event_handler_for_pf5 branch from c2c5baa to 60691e8 Compare July 18, 2024 13:13
@sgratch sgratch requested a review from yaacov July 18, 2024 13:29
@yaacov yaacov merged commit 86e4372 into kubev2v:main Jul 18, 2024
7 checks passed
@sgratch sgratch deleted the handle_rest_of_event_handler_for_pf5 branch July 18, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants