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

Extend Component class instead of using x-interaction #238

Merged
merged 13 commits into from
Feb 4, 2019

Conversation

dan-searle
Copy link
Contributor

@dan-searle dan-searle commented Jan 24, 2019

x-interaction unfortunately doesn't support our x-topic-search use case on the app, as it'll be re-rendered with any change in the followed topics array (on the MyFT -> Manage Topics page) and this causes us to run into #224.

Using a Preact Component class allows the consumer to manage and pass-in the followedTopicIds as a prop. x-topic-search holds in its state the topics just searched, and can then render the correct result depending on the followedTopicIds prop - e.g. present the search results, or show a "you already follow {topic}` message.

This also allows us to nicely encapsulate the body event listeners required to close the topic search results when you click elsewhere on the page (#239).

@dan-searle dan-searle mentioned this pull request Jan 30, 2019
@dan-searle dan-searle force-pushed the x-topic-search-as-class branch from 2e92c17 to 83373a1 Compare January 30, 2019 15:12
@dan-searle
Copy link
Contributor Author

This branch now includes cherry-pick of #241

if (targetIdIndex > -1) {
followedTopicIds.splice(targetIdIndex, 1);
handleInputClickOrFocus() {
if (this.state.searchTerm.length >= this.minSearchLength) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dan-searle This line will prevent the bug which it displays results even if there is no searchTerm?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not. This only happens when you click into the input. The bug happens when the input already has focus, a request has been made for suggestions, and the response arrives after the results container has been closed (because the input length < 2)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I know we are going to fix the bug after merging this PR, aren't we. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the fix is the resultsForTerm that's in various places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I had to revert that way of fixing it, as it didn't work very well. There's a different way of fixing it now.

Copy link
Contributor

@benbarnett benbarnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Just a thought about the catch block.

components/x-topic-search/src/ResultContainer.jsx Outdated Show resolved Hide resolved
components/x-topic-search/src/lib/get-suggestions.js Outdated Show resolved Hide resolved
components/x-topic-search/stories/knobs.js Show resolved Hide resolved
@dan-searle dan-searle force-pushed the x-topic-search-as-class branch from 881a6c4 to 80c2b45 Compare February 1, 2019 11:44
@dan-searle dan-searle merged commit 5ec60bc into x-topic-search Feb 4, 2019
@dan-searle dan-searle deleted the x-topic-search-as-class branch February 4, 2019 14:26
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.

3 participants