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

React: Navigator resetPageStack pop option #2284

Merged
merged 2 commits into from Dec 22, 2017

Conversation

Projects
None yet
2 participants
@zabojad
Copy link
Contributor

commented Dec 11, 2017

Following discussions on #2277 with @frandiox, this PR adds a pop option to Navigator.resetPageStack().

@zabojad zabojad changed the base branch from navigator-reset-pop to master Dec 11, 2017

@frandiox frandiox changed the title Navigator resetPageStack pop option React: Navigator resetPageStack pop option Dec 13, 2017

@zabojad

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

@frandiox can this be merged or is there anything wrong?

@frandiox

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

@zabojad Hey! I made a comment on a commit, didn't you see it?

@zabojad

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2017

Hey @frandiox ! On which commit have you made a comment ? I do not see it... Is it on
a62f9f9 or a3cbc5a ?

@frandiox

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2017

@zabojad Hmm, maybe I'm the only one who can see it 😅

@zabojad Doesn't this (this.pages = routes.map(renderPage)) need to be called for options.pop as well? I guess that's why you moved var renderPage to the top?

@zabojad

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2017

@frandiox it is one of the reasons I did ask you recently who is in charge of the React bindings in your team. Some of the OnSenUI React components sources I've crossed are strange (if not wrong) with things like calls to forceUpdate() which is not recommended (setState() should be used instead) or with variables that do not seem to be used at all. Here, in the Navigator component, the this.pages variable is anyway useless/never read because the render() method will anyway regenerate the pages from the routes variable.

So I kept this this.pages = routes.map(renderPage) in resetPageStack() as it is done in the other methods as well but it is actually never used.

I think the React bindings might need some refactoring / rewriting to match both React programming good practices as well as OnSenUI components implementation...

For now, I can put back down this line which should not have gone up. We can discuss the general implementation of the react bindings in another thread... What do you think?

@frandiox frandiox merged commit 1342456 into OnsenUI:master Dec 22, 2017

@frandiox

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

I think the React bindings might need some refactoring / rewriting to match both React programming good practices as well as OnSenUI components implementation...
For now, I can put back down this line which should not have gone up. We can discuss the general implementation of the react bindings in another thread... What do you think?

@zabojad Yes, I agree, it needs refactoring. I already did some but not enough. Feel free to open a new issue/PR and suggest modifications 👍

@zabojad

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2017

@frandiox well the good thing here is that it can be done component by component so it's completely do-able...

I'll open new PR-s soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.