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.

Sign up for GitHub

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

Jump to bottom

X topic search #218

Merged
merged 71 commits into from
Jan 31, 2022
Merged

X topic search #218

merged 71 commits into from
Jan 31, 2022

Conversation

fenglish
Copy link
Contributor

@fenglish fenglish commented Nov 12, 2018 •
edited
Loading

This is for x-dashing topic search bar on myFT page.

screen shot 2018-11-12 at 14 41 36

@i-like-robots
Copy link
Contributor

More components \0/

If you need a steer with this particular component I can help, I've built typeahead components a few times now!

dan-searle
dan-searle reviewed Nov 15, 2018
View reviewed changes
components/x-topic-search/readme.md Outdated Show resolved Hide resolved
components/x-topic-search/src/ResultContainer.jsx Outdated Show resolved Hide resolved
components/x-topic-search/src/TopicSearch.jsx Outdated Show resolved Hide resolved
@dan-searle dan-searle added the component A new component in development label Nov 16, 2018
@dan-searle
Copy link
Contributor

I think we need to consider if followedTopics needs to be updated during the component's lifecycle. E.g. as user follows or unfollows topics.

@dan-searle
Copy link
Contributor

Jest snapshot tests of the suggestions, no-suggestions and all-followed scenarios would be really useful too.

Good job so far 👍

@fenglish
Copy link
Contributor Author

fenglish commented Nov 16, 2018 •
edited
Loading

@dan-searle Thank you!!

Jest snapshot tests of the suggestions, no-suggestions and all-followed scenarios would be really useful too.

I'm working on that ^^

but ... I think it is difficult to do that... I thought I could do that using knobs and fetchMock but it doesn't pass searchTerm to the component so It can't create knobs for searchTerm, and also it fires api requests to get topics only on change.

@dan-searle
Copy link
Contributor

dan-searle commented Nov 19, 2018 •
edited
Loading

@fenglish have a look at the x-teaser-timeline unit tests here: https://github.com/Financial-Times/x-dash/pull/204/files#diff-573aecfd17ec8013aa4d6804eda8694bR22 These are separate to the snapshot tests that happen automatically for story definitions.

You should be able to do something similar and mock the responses from the search topics service to create the no-suggestions and all-followed scenarios.

@dan-searle dan-searle mentioned this pull request Jan 24, 2019
Open
@dan-searle
Copy link
Contributor

Looking good.

Are we going to update #228 and merge that into here?

Also, it would be good to update this to a class component #238 before we release this on the web or app.

@fenglish
Copy link
Contributor Author

fenglish commented Jan 30, 2019 •
edited
Loading

Do we need to apply #238 for x-topic-search on FT.com?
I would prefer to release x-topic-search before the change and merge the PR https://github.com/Financial-Times/next-myft-page/pull/1949 now, because the PR is getting bigger and some changes are needed on the consumer side if we apply #238.

@fenglish fenglish mentioned this pull request Jan 30, 2019
Open
@benbarnett benbarnett removed their request for review July 30, 2019 13:11
@edds
Copy link
Contributor

edds commented Oct 22, 2019

This PR is being closed due to inactivity as part of a Pull Request Bankruptcy. If you feel it’s still useful or relevant please feel free to re-open it.

@edds edds closed this Oct 22, 2019
@rowanbeentje rowanbeentje reopened this Jan 9, 2020
Base automatically changed from master to main February 8, 2021 13:05
@emortong emortong requested a review from a team as a code owner February 8, 2021 13:05
@fenglish fenglish requested a review from a team November 24, 2021 09:51
dan-searle and others added 22 commits January 31, 2022 15:29
@dan-searle @apaleslimghost
Slightly more descriptive tests.
447dea8
@dan-searle @apaleslimghost
Remove redundant throw in catch.
97be6f1
@dan-searle @apaleslimghost
Bug fix: csrfToken is a prop not state...
b5996bb
@dan-searle @apaleslimghost
Extract arrayToSentence function to be a component.
45181e5
@dan-searle @apaleslimghost
Don't show any search results if the searchTerm length is less than t…
85d16af 
…he minSearchLength.
@dan-searle @apaleslimghost
Remove "you already follow..." messaging and just display any suggest…
9adec50 
…ions with their follow buttons in the appropriate states.
@fenglish @apaleslimghost
Overwrite top/bottom padding 0 to keep input height the same across b…
1c3edc0 
…rowsers

It seems Safari includes the height of search cancel button as the input height and Chrome doesn't. Overwrite padding top/bottom 0 and keep the input height the min-height(40px) no matter whether the browser includes the cancel button height as the input height or not.
@fenglish @apaleslimghost
Remove o-forms
cc8ddb4 
The design doesn't have much commonality with o-forms and many of the styles are being overwritten.
@fenglish @apaleslimghost
Select the text when input box is changed from blur to focus
396d0db
@fenglish @apaleslimghost
Delete outdated explanation
55f0a05
@fenglish @apaleslimghost
Delete spaces
2a4a01d
@fenglish @apaleslimghost
Set more specific key instead of map index
93d1606
@fenglish @apaleslimghost
Update README
0ba9879
@dan-searle @apaleslimghost
Correct left alignment of no-suggestions bullet list.
9a1297a
@rowanbeentje @apaleslimghost
Unlock x-follow-button version to include compatible versions
4f40d01
@rowanbeentje @apaleslimghost
Bump x-follow-button dependency of x-topic-search to ^0.0.12
13dcebc
@apaleslimghost
AT-2015: Use a render prop for the follow button to make it work with…
b0149e1 
… x-follow-button 0.0.12
@apaleslimghost
AT-2015: Added prop docs
a0f64a5
@rowanbeentje @apaleslimghost
Miggrate x-topic-search Storybook setup to v6 format
0d6ba09
@rowanbeentje @apaleslimghost
Update x-topic-search bower dependencies to majoor versions used acro…
0fb6614 
…ss the rest of x-dash
@rowanbeentje @apaleslimghost
Update x-topic-search to use the merged component version of x-follow…
9b96d24 
…-button instead of the prerelease branch
@rowanbeentje @apaleslimghost
Update x-topic-search test setup so that fetchMock is set up in the b…
8c7f58c 
…eforeEach() blocks, to allow afterEach() to tear it down after each use instead of the at-init setup in each describe() block
@apaleslimghost apaleslimghost temporarily deployed to x-dash-x-topic-search-qprucz6r January 31, 2022 16:15 Inactive
@apaleslimghost apaleslimghost temporarily deployed to x-dash-x-topic-search-qprucz6r January 31, 2022 16:40 Inactive
@apaleslimghost apaleslimghost merged commit 076490d into main Jan 31, 2022
@apaleslimghost apaleslimghost deleted the x-topic-search branch January 31, 2022 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dan-searle dan-searle dan-searle left review comments

@apaleslimghost apaleslimghost apaleslimghost approved these changes

@i-like-robots i-like-robots Awaiting requested review from i-like-robots

Assignees
No one assigned
Labels
component A new component in development
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fenglish @i-like-robots @dan-searle @edds @apaleslimghost @rowanbeentje