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

Changed the search component in theme-gmd to use the event's currentTarget propery over the target to obtain the updated query. #799

Merged
merged 4 commits into from
Aug 12, 2019

Conversation

DanielRuf
Copy link
Contributor

@DanielRuf DanielRuf commented Aug 9, 2019

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.

@DanielRuf DanielRuf changed the base branch from v6.8.0 to v6.7.0 August 9, 2019 09:28
@DanielRuf DanielRuf changed the title Fix/gmd search onclick target 6 7 0 Use currentTarget for the element with the event listener (6.7.0) Aug 9, 2019
@SG-Noxoreos
Copy link
Contributor

SG-Noxoreos commented Aug 9, 2019

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.

In the current state the portal prop onClick, passed to the SEARCH_SUGGESTION_ITEM portal is not the actual handleSubmit, but is wrapped by a function that automatically fills out the query with the original suggestion entry. This was done to simplify its usage by not needing to pass the query to the handler, but changing the suggestion string/modifying the query is not accounted for, yet.

SInce you seem to need that I would suggest modifying the file themes/theme-ios11/pages/Browse/components/SearchField/components/SuggestionList/index.jsx in line 61 and change the code:

  • from onClick: e => onClick(e, suggestion),
  • to onClick: (e, q) => onClick(e, q && typeof q === 'string' ? q : suggestion),
    This will ensure that a query is always passed to the handleSubmit function, while it can still be overridden if needed.

@SG-Noxoreos SG-Noxoreos added the bug Something isn't working label Aug 9, 2019
@DanielRuf
Copy link
Contributor Author

Im currently trying onClick={onClick} ' but so far it differs between ios and gmd theme so it does not work to call onClick, e, text) by default (we set a different phrase). It should be completely passed through so we can use it like in the gmd theme. Same component code work in gmd but not ios theme.

@DanielRuf
Copy link
Contributor Author

But this is not the problem described in this PR. The problem is the wrong target. Let's discuss the other issue when this is handled (separate issue, already reported in the Slack channel).

@DanielRuf
Copy link
Contributor Author

As already written in the Slack channel onClick: onClick would be correct as it seems.

@SG-Noxoreos
Copy link
Contributor

As already written in the Slack channel onClick: onClick would be correct as it seems.

It would not fit our requirements to make the query optional. My suggested code change does fit that requirement and leaves room to modify the query as well.

But this is not the problem described in this PR. The problem is the wrong target. Let's discuss the other issue when this is handled (separate issue, already reported in the Slack channel).

Of course you can deal with this in a different scope.

@SG-Noxoreos SG-Noxoreos changed the title Use currentTarget for the element with the event listener (6.7.0) Changed the search component in theme-gmd to use the event's currentTarget propery over the target to obtain the updated query. Aug 12, 2019
@SG-Noxoreos SG-Noxoreos merged commit ecd46fd into shopgate:v6.7.0 Aug 12, 2019
@DanielRuf DanielRuf deleted the fix/gmd-search-onclick-target-6-7-0 branch August 12, 2019 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants