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

Repair stencil-route-prompt (ask user for confirmation before navigation) #121

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michael-speed-elder
Copy link

@michael-speed-elder michael-speed-elder commented Dec 7, 2020

The goal of this PR is to require confirmation before navigation under certain circumstances. From reading the code, it is clear this was almost a supported mechanism, and only minor tweaks are required to achieve that functionality through the <stencil-route-prompt> component.

When createBrowserHistory or createHashHistory runs, getUserConfirmation defaults to the getConfirmation function, which expects the global window object as its first parameter. However, it is later invoked without passing window, probably because the signature for the user-defined flavor in CreateBrowserHistoryOptions does not expect window to be passed.

The simple fix is to bind window to the default getConfirmation function as its first parameter.

I also have doc files written for the wiki: wiki-changes.zip

const getUserConfirmation =
props.getUserConfirmation != null
? props.getUserConfirmation
: getConfirmation.bind(undefined, win);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure window is passed as first parameter to getConfirmation.

@@ -103,7 +104,7 @@ const createBrowserHistory = (win: Window, props: CreateBrowserHistoryOptions =
);
};

const handlePopState = (event: any) => {
const handlePopState = (event: PopStateEvent) => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misc. clean up discovered while reading through code. Seemed like a specific type would be better.

@@ -52,7 +52,7 @@ const createHashHistory = (win: Window, props: CreateHashHistoryOptions = {}) =>
const keyLength = (props.keyLength != null) ? props.keyLength : 6;

const {
getUserConfirmation = getConfirmation,
getUserConfirmation = getConfirmation.bind(undefined, win),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure window is passed as first parameter to getConfirmation.

@michael-speed-elder michael-speed-elder changed the title fix(): bind window as first param of getConfirmation Repair stencil-route-prompt (ask user for confirmation before navigation) Dec 7, 2020
When `create***History` runs, `getUserConfirmation` defaults to the
`getConfirmation` function, which expects the global `window` object as
its first parameter.  However, it is later invoked without passing
`window`, probably because the signature for the user-defined flavor in
`CreateBrowserHistoryOptions` does not expect `window` to be passed.

The simple fix is to bind `window` to the default `getConfirmation`
function as its first parameter.
this.disable();
} else if (prevMessage !== newMessage) {
this.enable(this.message);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to simplify the logic here a little while being more earnest about the parameter types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant