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

fix(options menu): update examples to trigger select on the whole item #2513

Merged
merged 2 commits into from Jul 15, 2019

Conversation

@jenny-s51
Copy link
Contributor

jenny-s51 commented Jul 12, 2019

What: need feedback on these changes -- this WIP PR is in reference to #2415. Currently, the new click event handler I added seems to work fine on the first three options menu demos, where you can click anywhere on the item to select/deselect it.

However, the last three demos don't seem to work with this new prop, and when you click directly on the check mark of an item that's been selected, it still doesn't deselect the item.

Why would this be happening?

@jenny-s51 jenny-s51 changed the title WIP: fix(options menu): clickHandler needs review WIP: fix(options menu): new click handler needs review Jul 12, 2019
@jenny-s51 jenny-s51 changed the title WIP: fix(options menu): new click handler needs review WIP: fix(options menu): new click event handler needs review Jul 12, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jul 12, 2019

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

Copy link
Collaborator

jschuler left a comment

Hi @jenny-s51 , thank you for taking a look at this! I think the issue here is actually simpler, in the consumer onSelect callback function in OptionsMenu.md, we just simply have to change event.target.id to event.currentTarget.id.
The currentTarget refers to the element that the event listener is directly attached to while the target refers to the specific sub element that was clicked.

@jenny-s51

This comment has been minimized.

Copy link
Contributor Author

jenny-s51 commented Jul 12, 2019

@jschuler Ah wow, what a magical and simple fix! Now I see the difference between event.currentTargetand event.Target. Thank you for suggesting this! Will now update this PR to include it.

@jenny-s51 jenny-s51 requested review from jschuler and redallen Jul 12, 2019
@jschuler jschuler changed the title WIP: fix(options menu): new click event handler needs review fix(options menu): update examples to trigger select on the whole item Jul 12, 2019
Copy link
Collaborator

jschuler left a comment

Went ahead and removed the breaking change label and updated the title @jenny-s51

@jenny-s51

This comment has been minimized.

Copy link
Contributor Author

jenny-s51 commented Jul 12, 2019

@jschuler Awesome, I appreciate it -- thank you for approving this PR!

Copy link
Contributor

jessiehuff left a comment

LGTM! :)

@redallen redallen merged commit a533a7c into patternfly:master Jul 15, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jul 15, 2019

Your changes have been released in:

  • @patternfly/react-charts@4.6.1
  • @patternfly/react-core@3.72.1
  • @patternfly/react-docs@4.8.77
  • @patternfly/react-inline-edit-extension@2.9.40
  • demo-app-ts@2.12.2
  • @patternfly/react-table@2.14.14
  • @patternfly/react-topology@2.6.11
  • @patternfly/react-virtualized-extension@1.1.73

Thanks for your contribution! 🎉

@jenny-s51 jenny-s51 deleted the jenny-s51:iss2415 branch Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.