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

Search - extra tabs #3408

Merged
merged 11 commits into from
Apr 10, 2024
Merged

Search - extra tabs #3408

merged 11 commits into from
Apr 10, 2024

Conversation

mozzius
Copy link
Member

@mozzius mozzius commented Apr 4, 2024

This PR adds the additional Top/Latest tabs, behind a feature gate. The new tabs just have the same content, because we don't have the new APIs yet. It also allows the tab names to be translated

Screenshot 2024-04-04 at 21 15 04

Copy link

github-actions bot commented Apr 4, 2024

Old size New size Diff
6.34 MB 6.34 MB 385 B (0.01%)

@mozzius mozzius marked this pull request as ready for review April 5, 2024 08:22
@mozzius mozzius requested a review from gaearon April 5, 2024 08:23
@mozzius
Copy link
Member Author

mozzius commented Apr 5, 2024

huh, commit b2727d9a isn't appearing. odd.

@mozzius mozzius changed the title Search improvements Search - extra tabs Apr 5, 2024
@gaearon
Copy link
Collaborator

gaearon commented Apr 5, 2024

Seeing some duplicate views when in the gate. Both empty states and loading states are duplicated.

Screenshot 2024-04-05 at 14 15 25

Also People tab comes up empty for me. We should populate it with the same content as current Users tab.

Screenshot 2024-04-05 at 14 17 08

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

see above

Copy link

github-actions bot commented Apr 5, 2024

Old size New size Diff
6.36 MB 6.36 MB 3.07 KB (0.05%)

@mozzius mozzius force-pushed the samuel/search-revamp branch from 840936e to 8be4b3f Compare April 5, 2024 13:27
@mozzius mozzius requested a review from gaearon April 5, 2024 13:39
Copy link
Collaborator

@gaearon gaearon 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 if nits are fixed.

I wonder if it might feel more natural to move all branching to one place. eg you can have sections be an array of { title: _(…), content: <Stuff /> } and then iterate over those. Then there’s no risk that the titles will get out of sync with the content.

src/view/screens/Search/Search.tsx Outdated Show resolved Hide resolved
src/view/screens/Search/Search.tsx Outdated Show resolved Hide resolved
src/view/screens/Search/Search.tsx Outdated Show resolved Hide resolved
@haileyok
Copy link
Contributor

haileyok commented Apr 5, 2024

I don't mind this as far as UI goes, and this isn't something that needs to land in this PR, but I want to call out that we probably are going to want to add logic to only perform the search for the selected tab. Right now AFAIK the Posts tab and the Users tab both complete the search and render data in both tabs at once. This is going to be a performance issue on some devices if we start adding more tabs here (especially if it's a tab of posts).

Not sure what the logic we use in home is for this, but maybe similar?

@mozzius
Copy link
Member Author

mozzius commented Apr 5, 2024

That's a good shout, I'll look at that next week

@gaearon
Copy link
Collaborator

gaearon commented Apr 6, 2024

  1. Log out in the app
  2. Press search
  3. Type something
Screenshot 2024-04-06 at 02 06 35

Doesn't happen on main.

Tested with the gate set to fail.

<View>
<SearchScreenUserResults query={query} />
</View>
{sections.map(section => section.title)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You probably meant component here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Edited, this fixed the crash

<View>
<SearchScreenUserResults query={query} />
</View>
{sections.map(section => section.component)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the docs, its direct children have to be Views: https://github.com/callstack/react-native-pager-view?tab=readme-ov-file#usage

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

I think this works, let's merge after the next release

@gaearon gaearon merged commit 353a963 into main Apr 10, 2024
6 checks passed
estrattonbailey added a commit that referenced this pull request Apr 12, 2024
* origin/main: (455 commits)
  Use getSuggestions endpoint behind the gate (#3499)
  Added `new_profile_scroll_component` to `Gate` type (#3487)
  Fix useGate lint rule (#3486)
  Make bio area scrollable on iOS (#2931)
  Improve Android haptic, offer toggle for haptics in the app (#3482)
  Search - only enable queries once tab is active (#3471)
  [Statsig] Mark Testflight as staging tier (#3470)
  [Statsig] Typecheck gates (#3467)
  Bump to 1.77 (#3468)
  Search - extra tabs (#3408)
  notify slack on production builds (#3461)
  notify slack on production builds (#3460)
  1.76 release preparations (#3459)
  Update zh-CN translation (#3433)
  Italian Localization (#3388)
  [Statsig] Send prev route name (#3456)
  [Statsig] Instrument feed display (#3455)
  Small logic cleanups (#3449)
  Use ALF for the embed consent modal (#3336)
  Onboarding tweaks (#3447)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants