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

MainTabsScreen: Add Users icon to bottom nav #5506

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

manila
Copy link

@manila manila commented Sep 26, 2022

Fixes: #5495

This the start of a proposed fix for issue #5495

ios-users-tiny

TODO:

  • Performance testing
  • Test edge cases
  • Implement search
  • Test on Android
  • Test on iOS 12
  • Test on smaller screens

@manila
Copy link
Author

manila commented Sep 26, 2022

WIP

Not ready for review yet, feedback or suggestions are welcome anytime :D

@manila manila force-pushed the manila-issue-5495-pr branch 3 times, most recently from 31b821e to 2db36ec Compare September 26, 2022 08:53
@manila manila force-pushed the manila-issue-5495-pr branch from 2db36ec to 83313ce Compare September 26, 2022 09:01
@alya alya mentioned this pull request Oct 19, 2022
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks @manila! Code comments below.

In the UI, let's also have an app bar / nav bar at the top, with the title "Users". See SubscriptionsScreen for how we do that on the screen you get from the existing "#" tab.

Comment on lines +13 to +18
export default function UsersInfoScreen(props: Props): Node {
return (
<SafeAreaView mode="padding" edges={['top']} style={{ flex: 1 }} scrollEnabled={false}>
<UsersProfileCard filter="" />
</SafeAreaView>
);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having a trivial wrapper component like this, let's have just one component that does the job of both this and the inner component.

(I see the existing UsersScreen looks this way too. Maybe I'll go and fix that now.)

Copy link
Member

Choose a reason for hiding this comment

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

(I see the existing UsersScreen looks this way too. Maybe I'll go and fix that now.)

#5540

import UsersProfileCard from './UsersProfileCard';

type Props = $ReadOnly<{|
navigation: AppNavigationProp<'users'>,
Copy link
Member

Choose a reason for hiding this comment

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

This should follow the model of the other screen components that have the same navigation-related context -- which means the ones that also appear in MainTabsScreen in the same way that you're adding this one there. Take a look at the navigation props on those.

navigation: AppNavigationProp<'users'>,
|}>;

export default function UsersInfoScreen(props: Props): Node {
Copy link
Member

Choose a reason for hiding this comment

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

nit: function name of default export should match filename

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 high-priority redesign Belonging to redesign project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a "Users" panel
2 participants