Skip to content
This repository has been archived by the owner on Jul 15, 2019. It is now read-only.

Add replacestate link type #91

Merged
merged 5 commits into from
Apr 21, 2015
Merged

Add replacestate link type #91

merged 5 commits into from
Apr 21, 2015

Conversation

tbo
Copy link

@tbo tbo commented Apr 5, 2015

In some use cases it is useful to change the url without reseting the scroll position. It solves #82.

@yahoocla
Copy link

yahoocla commented Apr 5, 2015

CLA is valid!

@ross-pfahler
Copy link

@lingyan
Copy link
Contributor

lingyan commented Apr 7, 2015

@tbo Thanks for your contribution!

I like it, and am thinking it could actually be applied to both replace state and push state. Like in your product size use case, your product person can also decide to use pushState instead of replaceState.

What do you think about making preserveScrollPosition and replaceState as NavLink props with both default to false, and stick with the DEFAULT/CLICK nav type? You will need to modify NavLink to pass these two options through here. Maybe saving them under options key, since params are more for route params. Then it will be available in RouterMixin as nav.options.

@tbo
Copy link
Author

tbo commented Apr 13, 2015

@lingyan Thanks for your suggestions. I added it. But instead of encapsulating preserveScrollPosition in an option object I passed it directly. I hope that is okay.

@lingyan
Copy link
Contributor

lingyan commented Apr 14, 2015

@tbo Thanks for updating it! Passing the params as props is totally what I wanted :)

Could you add the params to the NavLink doc?

Also, can we do the same for replaceState? So it can be passed in as another prop like this.props.replaceState? That way, we can keep the code DRY by removing the repeated code in https://github.com/tbo/flux-router-component/blob/master/lib/RouterMixin.js#L141-L149 and https://github.com/tbo/flux-router-component/blob/master/lib/RouterMixin.js#L153-L161. And we won't really need to introduce a new nav type.

        switch (navType) {
            case TYPE_CLICK:
            case TYPE_DEFAULT:
                historyState = {params: (nav.params || {})};
                if(this._enableScroll) {
                    if (!nav.preserveScrollPosition) {
                        resetScrollPosition();
                    } else {
                        historyState.scroll = {x: window.scrollX, y: window.scrollY};
                    }
                }
                pageTitle = nav.params && nav.params.pageTitle || null;
                if (nav.replaceState) {
                    this._history.replaceState(historyState, pageTitle, newState.route.url);
                } else {
                    this._history.pushState(historyState, pageTitle, newState.route.url);
                }
                break;
            case TYPE_POPSTATE:
               ...

@tbo
Copy link
Author

tbo commented Apr 20, 2015

@lingyan I removed the repeated code, but I opted to keep the additional nav type. What do you think about it? I also added your suggestions regarding NavLink.

@lingyan
Copy link
Contributor

lingyan commented Apr 21, 2015

@tbo Thanks a lot for the update. My two cents: I actually think it would be better (and easier to read) to just make some minor change to the previous commit, like this:

        switch (navType) {
            case TYPE_CLICK:
            case TYPE_DEFAULT:
            case TYPE_REPLACESTATE:        
                historyState = {params: (nav.params || {})};
                if(this._enableScroll) {
                    if (!nav.preserveScrollPosition) {
                        resetScrollPosition();
                    } else {
                        historyState.scroll = {x: window.scrollX, y: window.scrollY};
                    }
                }
                pageTitle = nav.params && nav.params.pageTitle || null;
                if ((navType === TYPE_REPLACESTATE) || nav.replaceState) {
                    this._history.replaceState(historyState, pageTitle, newState.route.url);
                } else {
                    this._history.pushState(historyState, pageTitle, newState.route.url);
                }
                break;
            case TYPE_POPSTATE:
                if (this._enableScroll) {
                    ...

WDYT?

@tbo
Copy link
Author

tbo commented Apr 21, 2015

@lingyan Done.

@lingyan
Copy link
Contributor

lingyan commented Apr 21, 2015

🚢 Thanks @tbo! There is a minor tweak to move the resetScrollPosition function out for performance best practice. I will fix it after merging.

lingyan added a commit that referenced this pull request Apr 21, 2015
Add replacestate link type
@lingyan lingyan merged commit 2267c1b into YahooArchive:master Apr 21, 2015
@lingyan lingyan removed the ready label Apr 21, 2015
@mridgway mridgway mentioned this pull request Apr 21, 2015
13 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants