Comments (11)
Have we end up discussing something yesterday? From Slack discussion it's unclear to me whether this is still a bug and will get re-implemented.
from react-navigation.
@grabbou we didn't reach any conclusion. there's a small document which you should read and comment your opinion.
from react-navigation.
I'm a fan of #1 but happy to discuss. I like that option because there are no options to the screen config, just defaults. This is a simpler approach IMO
from react-navigation.
I am leaning towards the last approach which seems to be a natural progression of approach 2. Wiith a signature like this:
header: (navigation, header, params)
Personally I think it might be easier for user to have an explicit access to defaults that are loaded from parent source and params (options) that were explicitly passed to the function, when e.g. accessing an icon
for a focused state.
Not really sure if merging these two kinds of objects is a good idea since at some point, e.g. one might want to pass an option that is not part of navigationOptions.tabBar
and this will make people confused.
The biggest problem that I see with merging these two objects is that there's that assumption (at least I have it, I might be wrong) that options
(second parameter) are options that I specify, either on component inside static navigationOptions
or as defaults per navigator. Merging them with arbitrary params that library passes breaks that assumptions, since it's not me who is responsible for specifying these values.
from react-navigation.
In your example 2 the user would expect the options to be:
{
foo: 'bar',
tintColor: 'blue',
}
Correct?
from react-navigation.
In your example 2 the user would expect the options to be:
Only if the user returns a merged object,
from react-navigation.
I think I don't understand very well the problem described in this issue. I think the code example 2 at the beginning illustrates what's wrong currently but I don't understand it very well.
From the description:
Currently the second argument received by a navigation options item is the previous config returned from a parent navigation options, and the options passed to getScreenConfig.
How can the second argument be two things at once: (the previous config returned from a parent navigation options) and (the options passed to getScreenConfig)?
For example, the tab navigator can provide a focused argument for the icon, which doesn't make sense in a config.
OK this I understand. Yes that's true. But maybe it doesn't matter we merge those two things into a single options
object? Is this an issue just because it's difficult to declare good Flow types for the options?
Personally I think the confusing behaviour and the learning curve associated with it is not worth getting rid of one extra function.
Do you mean learning curve for contributors or for users of the library? There are many more users of the library so if the high-level API is simpler but as a tradeoff the implementation / Flow types are more complex that's an OK tradeoff. Ideally both API and implementation should be simple of course when possible.
The issue title says it's broken. What's the bug? Is there a bug in the Example 2 that the tintColor
set on the navigator gets lost?
In Proposal 1:
header: (navigation, options) => ({
...options,
icon: <Icon name={'settings' + options.focused ? '' : '-outline'} color={options.tintColor} />
});
- Why pass
navigation
? It's not used. When would it be used? I guess for rendering a right button that can navigate for example, right? - It's not ideal the user has to repeat
...options
- this is just some boilerplate to write. Can theicon
option be merged automatically with the navigator's options?
Proposal 2: I don't understand from the code examples how it's different from 1?
Proposal 3:
header: (navigation, params, previousResult) => ({
...previousResult,
icon: <Icon name={'settings' + params.focused ? '' : '-outline'} color={params.tintColor} />
});
What is previousResult
and how is it different from the option
in Proposal 1? As a user I just have to copy it as ...previousResult
but other than that it doesn't seem to have a purpose. Can the library do this boilerplate work so I can write:
header: (navigation, options) => ({
icon: <Icon name={'settings' + options.focused ? '' : '-outline'} color={options.tintColor} />
});
And the React Navigation library would do the merging of screen-navigationOptions and navigator-navigatorOptions for me. Basically it would work correctly without any API change.
from react-navigation.
Why pass navigation? It's not used. When would it be used?
It can be used for back button, or any button that changes navigation state such as updating params etc.
It's not ideal the user has to repeat ...options - this is just some boilerplate to write. Can the icon option be merged automatically with the navigator's options?
We had a discussion on slack regarding this https://reactnativecore.slack.com/archives/navigation/p1485457433002182
Proposal 2: I don't understand from the code examples how it's different from 1.
It has different config options, for example instead of a tintColor
and focused
argument, it has activeIcon
and inactiveIcon
, and getScreenConfig
doesn't pass any options.
What is previousResult and how is it different from the option in Proposal 1
previous result is
type HeaderOptionItemConfig = {
title: string;
icon: IconOptions => React.Element<*>;
options is
type IconOptions = {
focused: boolean;
tintColor: boolean;
}
options in proposal 1 is
HeaderOptionItemConfig & ?IconOptions
How can the second argument be two things at once: (the previous config returned from a parent navigation options) and (the options passed to getScreenConfig)?
Because they are merged together
HeaderOptionItemConfig & ?IconOptions
Just from reading this issue to me it seems like an API like this would work too?
This will work as long as the user returns a merged object. Also if the library merges them automatically, it doesn't need to be passed in the arguments. And merging these 2 and passing in argument means what I wrote in #1 which is confusing behaviour, merging options passed from the library with the header config defined in navigationOptions
.
from react-navigation.
Do you mean learning curve for contributors or for users of the library? There are many more users of the library so if the high-level API is simpler but as a tradeoff the implementation / Flow types are more complex that's an OK tradeoff. Ideally both API and implementation should be simple of course when possible.
I'm not worried about flow types being complex or API implementation. I'm more opposed to the confusing behaviour. It allows users to do strange things which don't make sense, e.g.-
tabBar: (navigation, options) => ({
focused: true,
})
Can you guess what this does?
I think what @ericvicenti wants is to treat options passed to getScreenConfig
the default navigationOptions
, but we're not using options that way, and probably default config there won't make sense since we're using getScreenConfig
in many places and we can't pass default config everywhere. The default config is already specified as component's default props.
from react-navigation.
Closing since we decided not to change any current behaviour.
from react-navigation.
Reopening as #984 will close it in a bit.
from react-navigation.
Related Issues (20)
- Sometimes duplicate tab bar labels appear in bottom tabs HOT 1
- unmountOnBlur shows white screen randomly when switchint tabs HOT 2
- useHeaderHeight changes several times when orientation changes HOT 3
- Navigate into page, increase RAM, navigate back and RAM stays high HOT 1
- When using frosted glass stack to jump in Android mode, ReactNavigation will experience lag and frosted glass will reset HOT 5
- Weird transition in navigation HOT 2
- Invariant Violation: requireNativeComponent: "RNSModalScreen" was not found in the UIManager. HOT 17
- [Bridgeless] [RN 0.74] Event cannot be both direct and bubbling: topFocus HOT 4
- Strange flicker when you scroll up a modal with NativeStack in iOS
- Ripple effect shown twice after clicking one time on material top tab after new architecture enabled. HOT 5
- onStateChange called after screen's mount (useEffect) HOT 6
- MaterialTopTabNavigator Icon Layout Shifting HOT 4
- To prevent the default behavior using e.preventDefault() on `tabPress` event is no longer working HOT 4
- RemoveEventListener error in react-native-navigation@4 HOT 7
- Keyboard is dismissed immediately when focusing on TextInput field on non-outer tabs for `react-native-tab-view` HOT 1
- RemoveEventListener error in react-native-navigation@4 HOT 8
- Bottom Tabs flash when opening keyboard HOT 2
- Bottom Tab Navigator falls out of viewable area HOT 2
- Wrong header height value for screens inside Native Stack inside another Native Stack with modal presentation on iOS
- headerLargeTitle: true with headerBackTitleVisible: false makes back button animation jerky HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from react-navigation.