KEMBAR78
router: warn user if there are unsaved edits when navigating away by yatbear · Pull Request #5223 · tensorflow/tensorboard · GitHub
Skip to content

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