Skip to content

Allow developers set the default clearHistory and backstackVisible options when the page is mounted in a frame #514

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

Merged
merged 1 commit into from
Jul 10, 2019

Conversation

msaelices
Copy link

See #513

…and backstackVisible options when the page is mounted in a frame.
@msaelices msaelices requested a review from rigor789 July 1, 2019 22:47
@msaelices
Copy link
Author

@rigor789 if you are ok with the change, I will adapt the current docs to reflect this change.

@rigor789
Copy link
Member

rigor789 commented Jul 6, 2019

I'm not sure if using a dynamic component inside a <Frame> is a good idea - but I'm fine with the changes to allow at least specifying the backstackVisible and clearHistory options.

Maybe it would be better to only expose a single prop, which allows passing any of the navigation options (this will allow future options to be passed as well).

So <Frame :navigationOptions="{ clearHistory: true, someFutureOption: false }"> would get merged in using Object.assign({}, this.navigationOptions).

What do you think?

@msaelices
Copy link
Author

@rigor789 I totally agree with you but, when I reviewed the frame.js code I realized that there were already some navigation options considered (see this code fragment) so I kept the same approach and I added some more navigation options.

So, If we change the approach we have to decide if:

a) we remove the current transition options adding them in the navigationOptions prop, which would add a backward incompatible change

b) we add the navigationOption but we keep the current transition one, mixing both approaches which is a little confusing but backward compatible.

@rigor789
Copy link
Member

rigor789 commented Jul 10, 2019

I'm happy with option a for 2.4.0 since that will be a 6.0 compatible release (will release it today if I manage to finish everything I have planned). I think I will leave this feature out of the current 2.3 (rc) release.

Edit: I think maybe we should keep the transition props, those are more commonly used afaik. Let's also go with the individual props for now - the only thing with this approach is that we need to maintain the props, but I don't think they'll change too often.

@rigor789 rigor789 changed the base branch from master to next July 10, 2019 13:54
@rigor789 rigor789 merged commit d06a7bd into next Jul 10, 2019
@msaelices msaelices deleted the improve-default-frame-navigation-options branch July 10, 2019 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants