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

Es/feat drawer navigation #351

Merged
merged 12 commits into from
May 27, 2024
Merged

Es/feat drawer navigation #351

merged 12 commits into from
May 27, 2024

Conversation

ErikSin
Copy link
Contributor

@ErikSin ErikSin commented May 14, 2024

Add drawer navigation

image

@ErikSin ErikSin requested a review from achou11 May 14, 2024 22:21
@ErikSin ErikSin force-pushed the es/feat-drawer-navigation branch from 3dcec73 to c13cd97 Compare May 14, 2024 22:21
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

Haven't read through implementation just yet, but here are some initial UI suggestions to make it closer to the intended design (see screenshots at bottom for frame of reference):

  • Increase font size and weight of all text items (should match design when using default OS system settings for fonts)

  • Grey box should extend over the safe area too

  • Use lighter shade of grey for grey box

  • horizontal spacing between the row icon and the text is a bit too large

  • maybe increase the horizontal padding around the text in the grey box? seems like the design has it so that the text never flows under the drawer icon. not as adamant/sure about this one though

  • Is there a drawer icon that matches the design? the one in the design has rounded ends whereas the one used in the implementation does not

    • currently

      image
    • design:

      image

  • are the tutorial row + items at the bottom of the drawer (about and privacy policy) intentionally excluded from the implementation for now? EDIT: not relevant, since this PR just ports the existing items, which is reasonable


What's currently implemented:

image

Expected design:

image

@ErikSin ErikSin force-pushed the es/feat-drawer-navigation branch from c13cd97 to 8d8b64d Compare May 21, 2024 19:05
@ErikSin
Copy link
Contributor Author

ErikSin commented May 21, 2024

  • horizontal spacing between the row icon and the text is a bit too large

This is a really annoying problem to fix. I think the < list /> actually needs to be refactored. The < ListIcon /> is hardcoded with a minSize but overwriting it from the parent component is actually quite annoying, you have to overwrite each instance of it.

So this is a temp solution that doesn't break all the other implementations of it.

@ErikSin
Copy link
Contributor Author

ErikSin commented May 21, 2024

  • Increase font size and weight of all text items (should match design when using default OS system settings for fonts)

i fixed the header size
For the other fonts, I'm just following the font size based of the notion style guide. I think if we do change it here, it needs to be changed everywhere. It is going into QA and the designers will see it, so if we need to increase the font size, it would probably be better to leave all as it (which is inherhited from the Text component), and just fix the text component accordingly.

@ErikSin
Copy link
Contributor Author

ErikSin commented May 21, 2024

  • Grey box should extend over the safe area too

  • Use lighter shade of grey for grey box

  • maybe increase the horizontal padding around the text in the grey box? seems like the design has it so that the text never flows under the drawer icon. not as adamant/sure about this one though

  • Is there a drawer icon that matches the design? the one in the design has rounded ends whereas the one used in the implementation does not

addressed via fb37bbf

Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

couple of smaller pieces of feedback but otherwise the implementation feels good!

src/frontend/Navigation/Drawer.tsx Outdated Show resolved Hide resolved
);
};

const DrawerContent = ({navigation}: DrawerContentComponentProps) => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: should create a stylesheet for this component and extract the inline styles out into it

src/frontend/Navigation/Stack/index.tsx Outdated Show resolved Hide resolved
src/frontend/Navigation/Stack/index.tsx Outdated Show resolved Hide resolved
src/frontend/sharedComponents/HomeHeader.tsx Outdated Show resolved Hide resolved
src/frontend/Navigation/Stack/AppScreens.tsx Outdated Show resolved Hide resolved
@ErikSin ErikSin merged commit 063c82c into main May 27, 2024
3 checks passed
@ErikSin ErikSin deleted the es/feat-drawer-navigation branch May 27, 2024 17:13
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.

2 participants