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

Use currentTarget for the element with the event listener #797

Closed
wants to merge 3 commits into from
Closed

Use currentTarget for the element with the event listener #797

wants to merge 3 commits into from

Conversation

DanielRuf
Copy link
Contributor

Description

This PR resolves the wrong target in #793. Alternatively we could also catch e ourself, set the needed value to the one of currentTarget and call the original onClick method with e.

Type of change

Please add an "x" into the option that is relevant:

  • Bug Fix 🐛 (non-breaking change which fixes an issue)
  • Enhancement 🚀 (non-breaking change which adds functionality)
  • Breaking Change 💥 (fix or feature that would cause existing functionality to not work as expected)
  • Polish 💅 (Just some cleanups)
  • Internal 🏠 Only relates to internal processes.

How to test it

Create any html elements in the button element for the search suggestion item and click on the element. It should correctly get the value of the element with the event listener, not one of its children.

@SG-Noxoreos SG-Noxoreos changed the base branch from v6.8.0 to v6.7.0 August 9, 2019 08:54
@SG-Noxoreos SG-Noxoreos changed the title Use currentTarget for the element with the eventlistener - fixes #793 Use currentTarget for the element with the event listener Aug 9, 2019
@SG-Noxoreos SG-Noxoreos changed the base branch from v6.7.0 to v6.8.0 August 9, 2019 08:56
@DanielRuf
Copy link
Contributor Author

Attention, simulate('eventname') will be deprecated and is not recommended.

enzymejs/enzyme#218
enzymejs/enzyme#1455
enzymejs/enzyme#1551
enzymejs/enzyme#1943
enzymejs/enzyme#2080
enzymejs/enzyme#2173

@SG-Noxoreos
Copy link
Contributor

SG-Noxoreos commented Aug 9, 2019

Looking at the issue #793, this change was actually requested for version 6.7.0 and not for 6.8.0. But this PR is based off and points to version branch v6.8.0 instead.

This topic is a bit bigger than what was addressed here, because the implementation of the SuggestionList differs in the themes and the ios implementation does not rely on the target value at all.
See: themes/theme-ios11/pages/Browse/components/SearchField/index.jsx :: handleSubmit.
This means, while the proposed change introduces a way to modify the search query, it is currently not possible to do so in the ios theme.

@DanielRuf
Copy link
Contributor Author

See: themes/theme-ios11/pages/Browse/components/SearchField/index.jsx :: handleSubmit.
This means, while the proposed change introduces a way to modify the search query, it is currently not possible to do so in the ios theme.

We can if the onClick portal prop is correctly set.
You are right, this change was planned / thought for 6.7.0 as we currently use 6.7.0-rc.4.

@DanielRuf
Copy link
Contributor Author

This means, while the proposed change introduces a way to modify the search query, it is currently not possible to do so in the ios theme.

This is not a problem at all there.

@DanielRuf
Copy link
Contributor Author

Replaced by #799

@DanielRuf DanielRuf closed this Aug 9, 2019
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.

2 participants