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

router: warn user if there are unsaved edits when navigating away #5223

Merged
merged 4 commits into from
Aug 11, 2021

Conversation

yatbear
Copy link
Member

@yatbear yatbear commented Aug 10, 2021

Warn user with window.confirm if there are unsaved edits when navigating away from the current route:

  • If user confirms, discards the edits and navigate to the new route;
  • Otherwise discontinue the dispatchNavigating$.

@yatbear yatbear requested a review from stephanwlee August 10, 2021 23:11
@google-cla google-cla bot added the cla: yes label Aug 10, 2021
@yatbear yatbear marked this pull request as draft August 11, 2021 00:07
@yatbear
Copy link
Member Author

yatbear commented Aug 11, 2021

Updated the local test screencast, clicking on back button seems to only work when the navigation is in app.

@yatbear yatbear marked this pull request as ready for review August 11, 2021 01:05
Comment on lines 26 to 28
on(actions.discardDirtyUpdates, (state) => {
return {...state};
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please undo this change; this is basically noop and we do not need to make changes to the app_routing state based on this action at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Thanks!

Comment on lines 337 to 341
expect(actualActions).toEqual([actions.discardDirtyUpdates()]);

tick();
expect(actualActions).toEqual([
actions.navigating({
Copy link
Contributor

Choose a reason for hiding this comment

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

I am surprised that the test is passing here. Shouldn't the actualActions be an array with two elements where the first element in the actions.dsicardDirtyUpdates()? Similarly, I expected the L352 to have three elements. What am I missing?

Copy link
Member Author

@yatbear yatbear Aug 11, 2021

Choose a reason for hiding this comment

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

I used fakeAsync incorrectly as below and it didn't capture the errors :(

it ('wrong test', () => {
  fakeAsync(() => {})
});

Fixed now.

@yatbear yatbear merged commit 7bf7e51 into tensorflow:master Aug 11, 2021
yatbear added a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
…nsorflow#5223)

* warn user if there are unsaved edits when navigating away from the current route

* fire discardDirtyUpdates action when user wants to discard unsaved changes
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
…nsorflow#5223)

* warn user if there are unsaved edits when navigating away from the current route

* fire discardDirtyUpdates action when user wants to discard unsaved changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants