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

Update types.tsx #11281

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

nikmantulenko
Copy link

I'm proposing to add better typing possibilities for custom header components

Motivation

using NativeStackHeaderProps type for 'options.header = (props: NativeStackHeaderProps) => ...
gives no type checking for route & navigation props

making NativeStackHeaderProps type generic
@github-actions
Copy link

Hey @nikmantulenko! Thanks for opening your first pull request in this repo. If you haven't already, make sure to read our contribution guidelines.

@netlify
Copy link

netlify bot commented Mar 15, 2023

Deploy Preview for react-navigation-example ready!

Name Link
🔨 Latest commit 3b84583
🔍 Latest deploy log https://app.netlify.com/sites/react-navigation-example/deploys/644a8b2f5c2d89000871fc68
😎 Deploy Preview https://deploy-preview-11281--react-navigation-example.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link

@branaust branaust left a comment

Choose a reason for hiding this comment

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

LGTM

@osdnk
Copy link
Member

osdnk commented Feb 23, 2024

Hi, @nikmantulenko

Thanks for the PR!

I think this might be a nice approach, but at this stage, this seems to be a very atomic change, whereas there are multiple places with a very similar approach implemented (to name a few, consider all components in the stack).

So if you are up to making the holistic migration, we will be happy to merge. Currently, we find this use case a bit rare (to have header parametrized by the route) so we will probably not spend the effort actively on implementing that. But, as stated earlier, we are happy to review the follow-up change.

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.

None yet

3 participants