🐾 Update to PF5 - part II - onSelect, onToggle event handlers #1221

sgratch · 2024-06-18T11:12:29Z

Reference: #1098

For avoiding uncaught PF 4 -> PF 5 migration errors, this PR named and typed (i.e. (add type to function signature) all onSelect, onToggle callbacks appearances which use parameters.

yaacov · 2024-06-19T05:15:13Z

packages/common/src/components/ActionServiceDropdown/ActionServiceDropdown.tsx

+  const onToggle: (
+    isExpanded: boolean,
+    event:
+      | Event
+      | React.KeyboardEvent<Element>
+      | React.MouseEvent<Element, MouseEvent>
+      | React.ChangeEvent<Element>,
+  ) => void = (isExpanded) => {
+    setIsActionMenuOpen(isExpanded);
+  };


Suggested change

const onToggle: (

isExpanded: boolean,

event:

| Event

| React.KeyboardEvent<Element>

| React.MouseEvent<Element, MouseEvent>

| React.ChangeEvent<Element>,

) => void = (isExpanded) => {

setIsActionMenuOpen(isExpanded);

};

// define once for all onToggle handlers

export type ToggleEvent =

| Event

| React.KeyboardEvent<Element>

| React.MouseEvent<Element, MouseEvent>

| React.ChangeEvent<Element>;

const onToggle: (isExpanded: boolean, event: ToggleEvent) => void = (isExpanded) => {

setIsActionMenuOpen(isExpanded);

};

yaacov · 2024-06-19T05:17:49Z

...s/forklift-console-plugin/src/modules/Providers/views/migrate/components/MappingListItem.tsx

@@ -52,6 +53,30 @@ export const MappingListItem: FC<MappingListItemProps> = ({
  const { t } = useForkliftTranslation();
  const [isSrcOpen, setToggleSrcOpen] = useToggle(false);
  const [isTrgOpen, setToggleTrgOpen] = useToggle(false);
+
+  const onSelectForSource: (


Suggested change

const onSelectForSource: (

const onSelectSource: (

yaacov · 2024-06-19T05:18:00Z

...s/forklift-console-plugin/src/modules/Providers/views/migrate/components/MappingListItem.tsx

+      });
+  };
+
+  const onSelectForDestination: (


Suggested change

const onSelectForDestination: (

const onSelectDestination: (

yaacov · 2024-06-19T05:29:40Z

packages/common/src/components/Filter/AutocompleteFilter.tsx

+  const onSelect: (
+    event: React.MouseEvent<Element, MouseEvent> | React.ChangeEvent<Element>,
+    value: string | SelectOptionObject,
+    isPlaceholder?: boolean,
+  ) => void = (event, value, isPlaceholder) => {
+    if (isPlaceholder) {
+      return;
+    }
+    // behave as Console's AutocompleteInput used i.e. to filter pods by label
+    if (!hasFilter(value)) {
+      addFilter(value);
+    }
+    setExpanded(false);
+  };


Suggested change

const onSelect: (

event: React.MouseEvent<Element, MouseEvent> | React.ChangeEvent<Element>,

value: string | SelectOptionObject,

isPlaceholder?: boolean,

) => void = (event, value, isPlaceholder) => {

if (isPlaceholder) {

return;

}

// behave as Console's AutocompleteInput used i.e. to filter pods by label

if (!hasFilter(value)) {

addFilter(value);

}

setExpanded(false);

};

export type SelectEvent = React.MouseEvent<Element, MouseEvent> | React.ChangeEvent<Element>;

export type SelectValue = string | SelectOptionObject;

const onSelect: (event: SelectEvent, value: SelectValue, isPlaceholder?: boolean) => void = (event, value, isPlaceholder) => {

if (isPlaceholder) {

return;

}

// behave as Console's AutocompleteInput used i.e. to filter pods by label

if (!hasFilter(value)) {

addFilter(value);

}

setExpanded(false);

};

yaacov · 2024-07-04T06:54:38Z

packages/common/src/components/Filter/AutocompleteFilter.tsx

+
+  const onToggle: (
+    isExpanded: boolean,
+    event:


Suggested change

event:

event: ToggleEvent

yaacov · 2024-07-04T06:59:05Z

packages/common/src/components/ActionServiceDropdown/ActionServiceDropdown.tsx

@@ -67,12 +67,25 @@ const ActionsAsDropdown = ({
  const history = useHistory();
  const [isActionMenuOpen, setIsActionMenuOpen] = useState(false);
  const isPlain = variant === 'kebab';
+
+  // define once for all onToggle handlers


Suggested change

// define once for all onToggle handlers

/**

* @typedef {Object} ToggleEvent

* @description Represents the possible event types that can be used for toggling actions.

*

* @property {Event} Event - A standard DOM event.

* @property {React.KeyboardEvent<Element>} React.KeyboardEvent - A React-specific keyboard event.

* @property {React.MouseEvent<Element, MouseEvent>} React.MouseEvent - A React-specific mouse event.

* @property {React.ChangeEvent<Element>} React.ChangeEvent - A React-specific change event.

*/

the define once for all onToggle handlers comment I suggested in my previous review was to help clarify why we need this type definition, when we actually write the code, we can do better. p.s. you did not use in in other onToggle handlers, please do use in in all onToggle handlers ( see https://github.com/kubev2v/forklift-console-plugin/pull/1221/files#r1665219359 )

I didn't like this fix since it manipulates the callbacks function signatures, but if you insist and think it is nicer then ok. I made the change in all relevant places now.

yaacov · 2024-07-07T13:43:36Z

packages/forklift-console-plugin/src/modules/Plans/views/details/components/MappingListItem.tsx

@@ -54,6 +55,29 @@ export const MappingListItem: FC<MappingListItemProps> = ({
    deleteMapping({ source, destination });
  };

+  const onSelectForSource: (


Suggested change

const onSelectForSource: (

const onSelectSource: (

yaacov · 2024-07-07T13:43:48Z

packages/forklift-console-plugin/src/modules/Plans/views/details/components/MappingListItem.tsx

+      });
+  };
+
+  const onSelectForDestination: (


Suggested change

const onSelectForDestination: (

const onSelectDestination: (

Reference: kubev2v#1098 For avoiding uncaught PF 4 -> PF 5 migration errors, this PR named and typed (i.e. (add type to function signature) all onSelect, onToggle callbacks appearances which use parameters. Signed-off-by: Sharon Gratch <[email protected]>

yaacov added the enhancement Categorizes issue or PR as related to a new feature. label Jun 18, 2024

yaacov added this to the 2.7.0 milestone Jun 18, 2024

yaacov changed the title ~~Update to PF5 - part II - onSelect, onToggle event handlers~~ 🐾 Update to PF5 - part II - onSelect, onToggle event handlers Jun 18, 2024

sgratch force-pushed the onSelect_onToggle_event_handler_for_pf5 branch from c944633 to 5763175 Compare June 18, 2024 22:57

sgratch requested a review from yaacov June 18, 2024 23:16

yaacov reviewed Jun 19, 2024

View reviewed changes

sgratch force-pushed the onSelect_onToggle_event_handler_for_pf5 branch 2 times, most recently from 9009859 to 2a8e1a3 Compare July 3, 2024 12:32

sgratch requested a review from yaacov July 3, 2024 12:58

yaacov reviewed Jul 4, 2024

View reviewed changes

sgratch force-pushed the onSelect_onToggle_event_handler_for_pf5 branch from 2a8e1a3 to 6776415 Compare July 4, 2024 12:47

sgratch requested a review from yaacov July 4, 2024 12:55

yaacov reviewed Jul 7, 2024

View reviewed changes

sgratch force-pushed the onSelect_onToggle_event_handler_for_pf5 branch from 6776415 to 590d459 Compare July 7, 2024 14:08

sgratch requested a review from yaacov July 7, 2024 14:16

yaacov merged commit c4a3a5c into kubev2v:main Jul 7, 2024
7 checks passed

yaacov mentioned this pull request Jul 22, 2024

🐾 Update to PF5 - part II #1098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐾 Update to PF5 - part II - onSelect, onToggle event handlers #1221

🐾 Update to PF5 - part II - onSelect, onToggle event handlers #1221

sgratch commented Jun 18, 2024

yaacov Jun 19, 2024

yaacov Jun 19, 2024

yaacov Jun 19, 2024

yaacov Jun 19, 2024

yaacov Jul 4, 2024

yaacov Jul 4, 2024

sgratch Jul 4, 2024

yaacov Jul 7, 2024

yaacov Jul 7, 2024

-  // define once for all onToggle handlers
+  /**
+ * @typedef {Object} ToggleEvent
+ * @description Represents the possible event types that can be used for toggling actions.
+ *
+ * @property {Event} Event - A standard DOM event.
+ * @property {React.KeyboardEvent<Element>} React.KeyboardEvent - A React-specific keyboard event.
+ * @property {React.MouseEvent<Element, MouseEvent>} React.MouseEvent - A React-specific mouse event.
+ * @property {React.ChangeEvent<Element>} React.ChangeEvent - A React-specific change event.
+ */

🐾 Update to PF5 - part II - onSelect, onToggle event handlers #1221

🐾 Update to PF5 - part II - onSelect, onToggle event handlers #1221

Conversation

sgratch commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment