Skip to content

Conversation

@quarckster
Copy link
Contributor

Part of #2423

What: Added OUIA props to Dropdown as per #2423

karelhala
karelhala previously approved these changes Oct 14, 2019
@quarckster
Copy link
Contributor Author

quarckster commented Oct 14, 2019

@redallen please help with type matching. Build failed due to this issue.

@karelhala
Copy link
Contributor

@quarckster looks like the problem was with incorrect onSelect type. This should do the trick.

diff --git a/packages/patternfly-4/react-core/src/components/Dropdown/Dropdown.tsx b/packages/patternfly-4/react-core/src/components/Dropdown/Dropdown.tsx
index 4cc061c65..86ee33ed3 100644
--- a/packages/patternfly-4/react-core/src/components/Dropdown/Dropdown.tsx
+++ b/packages/patternfly-4/react-core/src/components/Dropdown/Dropdown.tsx
@@ -23,7 +23,7 @@ export interface DropdownProps extends React.HTMLProps<HTMLDivElement> {
   /** Toggle for the dropdown, examples: <DropdownToggle> or <DropdownToggleCheckbox> */
   toggle: React.ReactElement<any>;
   /** Function callback called when user selects item */
-  onSelect?(event: React.SyntheticEvent<HTMLDivElement>): void;
+  onSelect?(event?: React.SyntheticEvent<HTMLDivElement>): void;
   /** Flag to indicate if the first dropdown item should gain initial focus, set false when adding
    * a specific auto-focus item (like a current selection) otherwise leave as true
    */
diff --git a/packages/patternfly-4/react-core/src/components/Dropdown/DropdownWithContext.tsx b/packages/patternfly-4/react-core/src/components/Dropdown/DropdownWithContext.tsx
index fe1ea2218..307e825e8 100644
--- a/packages/patternfly-4/react-core/src/components/Dropdown/DropdownWithContext.tsx
+++ b/packages/patternfly-4/react-core/src/components/Dropdown/DropdownWithContext.tsx
@@ -21,7 +21,7 @@ class DropdownWithContext extends React.Component<DropdownProps & InjectedOuiaPr
     isGrouped: false,
     position: DropdownPosition.left,
     direction: DropdownDirection.down,
-    onSelect: Function.prototype,
+    onSelect: (): void => undefined,
     autoFocus: true,
     ouiaComponentType: 'Dropdown'
   };

Also don't forget to run node_modules/.bin/jest -u packages/patternfly-4/react-core/src/components/ in order to update your snapshots.

@redallen
Copy link
Contributor

@quarckster I believe @karelhala is correct. This mismatching type for onSelect is Function vs (event: SyntheticEvent<HTMLDivElement, Event>) => void.

dlabrecq
dlabrecq previously approved these changes Oct 16, 2019
@dlabrecq dlabrecq dismissed their stale review October 16, 2019 01:56

Didn't see the mismatching type

@quarckster quarckster changed the title [WIP] Added OUIA compatibility for Dropdown Added OUIA compatibility for Dropdown Oct 16, 2019
@patternfly-build
Copy link
Collaborator

PatternFly-React preview: https://patternfly-react-pr-3135.surge.sh

@codecov-io
Copy link

codecov-io commented Oct 16, 2019

Codecov Report

Merging #3135 into master will decrease coverage by <.01%.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3135      +/-   ##
==========================================
- Coverage   68.98%   68.98%   -0.01%     
==========================================
  Files         858      858              
  Lines       23612    23621       +9     
  Branches     1887     1889       +2     
==========================================
+ Hits        16288    16294       +6     
- Misses       6364     6366       +2     
- Partials      960      961       +1
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.3% <ø> (ø) ⬆️
#patternfly4 67.93% <70%> (-0.01%) ⬇️
Impacted Files Coverage Δ
...-4/react-core/src/components/Dropdown/Dropdown.tsx 100% <ø> (ø) ⬆️
...re/src/components/Dropdown/DropdownWithContext.tsx 90.32% <70%> (-4.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73c6169...a7390a7. Read the comment docs.

@karelhala karelhala added the ouia label Oct 16, 2019
@karelhala
Copy link
Contributor

Looks like the table snapshots were not updated @quarckster can you run node_modules/.bin/jest -u packages/patternfly-4/ to fix the failing tests as well?

@quarckster
Copy link
Contributor Author

@redallen @karelhala @dlabrecq tests passed

@redallen redallen merged commit 8a9d853 into patternfly:master Oct 17, 2019
@patternfly-build
Copy link
Collaborator

Your changes have been released in:

  • @patternfly/react-core@3.116.2
  • @patternfly/react-docs@4.14.28
  • @patternfly/react-inline-edit-extension@2.11.97
  • demo-app-ts@3.7.11
  • @patternfly/react-table@2.23.13
  • @patternfly/react-topology@2.9.6
  • @patternfly/react-virtualized-extension@1.2.79

Thanks for your contribution! 🎉

@quarckster quarckster deleted the dropdown_ouia branch October 17, 2019 14:15
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.

6 participants