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

fix: undefined colors property for material bottom tab #11219

Closed
wants to merge 1 commit into from
Closed

fix: undefined colors property for material bottom tab #11219

wants to merge 1 commit into from

Conversation

FN-FAL113
Copy link

@FN-FAL113 FN-FAL113 commented Feb 12, 2023

Motivation

Fixes unsafe access to an undefined property during theme change while using material bottom tab
With this the developer still gets the freedom on theming their navigations while not using react-native-paper's provider context

resolves #11208

Test plan

  1. Toggle dark theme
  2. DefaultTheme, DarkTheme or any custom theme created by a developer is passed to NavigationContainer theme props
  3. An error gets thrown once a developer uses a different theme while using material bottom tab navigation
    Screenshot 2023-02-12 104831

@github-actions
Copy link

Hey @FN-FAL113! 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 Feb 12, 2023

Deploy Preview for react-navigation-example ready!

Name Link
🔨 Latest commit aa22897
🔍 Latest deploy log https://app.netlify.com/sites/react-navigation-example/deploys/63e85622550576000800995e
😎 Deploy Preview https://deploy-preview-11219--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.

@FN-FAL113
Copy link
Author

This fixes issues when a developer is not using react native paper's Provider wrapper/context

@FN-FAL113
Copy link
Author

FN-FAL113 commented Feb 12, 2023

Ok upon testing and debugging, DarkTheme being imported from react-native-paper would cause the said undefined issue.
This might be because DarkTheme got deprecated and changed to MD3DarkTheme or any other changes to react native paper's theme.
Changing DarkTheme to MD3DarkTheme inside the imports would also suffice as a fix for this issue but unsure of backwawrds compatibility or wrapping material bottom navigation component with paper's Provider context with a given theme prop
image

Let me know what route should this pr go.

@danieldanielecki
Copy link

It looks like this bug have been introduced in a83f6e0

@satya164
Copy link
Member

Thanks for the PR. It'll be problematic to do use MD3DarkTheme as it may break compatibility with older versions of paper.

You should instead make sure that paper and react navigation's themes match: e.g. pass dark theme for react navigation if you're using dark theme for paper.

https://callstack.github.io/react-native-paper/theming-with-react-navigation.html

@satya164 satya164 closed this Feb 12, 2023
@github-actions
Copy link

Hey! This issue is closed and isn't watched by the core team. You are welcome to discuss the issue with others in this thread, but if you think this issue is still valid and needs to be tracked, please open a new issue with a repro.

@FN-FAL113
Copy link
Author

FN-FAL113 commented Feb 13, 2023

Thanks for the PR. It'll be problematic to do use MD3DarkTheme as it may break compatibility with older versions of paper.

You should instead make sure that paper and react navigation's themes match: e.g. pass dark theme for react navigation if you're using dark theme for paper.

https://callstack.github.io/react-native-paper/theming-with-react-navigation.html

understood, but any reason why an optional chaining can't be implemented in-case a developer uses a different theme, this could prevent such exceptions being thrown that is not really informative for debugging.

@satya164
Copy link
Member

this navigator will move to react-native-paper package to support the new version so I don't want to add workarounds for it here.

@danieldanielecki
Copy link

@FN-FAL113, were you able to find in which release they expect to fix it?

@FN-FAL113
Copy link
Author

FN-FAL113 commented Feb 20, 2023

@FN-FAL113, were you able to find in which release they expect to fix it?

I resorted to using the paper context provider, I supplied a theme as a value

@FN-FAL113
Copy link
Author

Although I have not checked the latest version on react native paper if its fixed

@danieldanielecki
Copy link

@FN-FAL113, were you able to find in which release they expect to fix it?

I resorted to using the paper context provider, I supplied a theme as a value

can you provide code snippet?

@danieldanielecki
Copy link

Although I have not checked the latest version on react native paper if its fixed

@satya164, might you let us know?

@danieldanielecki
Copy link

Thanks for the PR. It'll be problematic to do use MD3DarkTheme as it may break compatibility with older versions of paper.

You should instead make sure that paper and react navigation's themes match: e.g. pass dark theme for react navigation if you're using dark theme for paper.

https://callstack.github.io/react-native-paper/theming-with-react-navigation.html

page not found...

@FN-FAL113
Copy link
Author

FN-FAL113 commented Mar 8, 2023

Thanks for the PR. It'll be problematic to do use MD3DarkTheme as it may break compatibility with older versions of paper.
You should instead make sure that paper and react navigation's themes match: e.g. pass dark theme for react navigation if you're using dark theme for paper.
https://callstack.github.io/react-native-paper/theming-with-react-navigation.html

page not found...

https://callstack.github.io/react-native-paper/docs/guides/theming
https://callstack.github.io/react-native-paper/docs/guides/theming-with-react-navigation

@danieldanielecki
Copy link

It's as simple as applying 2 lines of code:

import { MD3DarkTheme, Provider as PaperProvider } from "react-native-paper";
...
<PaperProvider theme={MD3DarkTheme}>
...

the docs confused me a lot.

@FN-FAL113
Copy link
Author

It's as simple as applying 2 lines of code:

import { MD3DarkTheme, Provider as PaperProvider } from "react-native-paper";
...
<PaperProvider theme={MD3DarkTheme}>
...

the docs confused me a lot.

if you read the wrong part it will surely confused you, It did not took me that long to fix the issue once I got to read the docs

@danieldanielecki
Copy link

Not sure why they include the CombinedDarkTheme part if it works simply with MD3DarkTheme.

@FN-FAL113
Copy link
Author

I guess its for combining navigation and paper theming? since paper serves their own components with compliant material designs and not just aimed for navigation theming

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.

Cannot read property colors of undefined @react-navigation/material-bottom-tabs
3 participants