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

Unsaved changes modal #252

Merged
merged 71 commits into from
Dec 18, 2018
Merged

Conversation

parostatkiem-zz
Copy link
Contributor

@parostatkiem-zz parostatkiem-zz commented Nov 27, 2018

This change definitely needs a backdrop support which should be provided with #352.
I left some comments with my suggestions about how Core's backdrop can be handled.

Changes proposed in this pull request:

  • Create new component called ConfirmationModal
  • Handle the modal showing and hiding in App.js and return a promise with the result
  • Trigger the modal in routing.js::handleRouteChange() if current page is dirty, then wait for result

Related issue(s)

Resolves #210

‼️ IMPORTANT ‼️

Don't forget to swap your Console repo to the branch from this PR:

@jesusreal jesusreal self-assigned this Nov 29, 2018
@pekura pekura self-assigned this Nov 30, 2018
@maxmarkus maxmarkus removed the WIP Work in progress label Dec 13, 2018
@pekura
Copy link
Contributor

pekura commented Dec 13, 2018

It does not work properly in preserveView/goBack scenario: when you are on a page where there is a go back possibility with preserved view and you try to go back using linkManager().goBack() and the current page is dirty the content goes back to the preserved view, the URL stays the same, and the unsaved changes modal pops up. Independent of what you choose (Yes or No) the situation won't get better.

@pekura
Copy link
Contributor

pekura commented Dec 13, 2018

Don't we want to have a backdrop for the unsaved changes modal?

@jesusreal
Copy link
Contributor

Don't we want to have a backdrop for the unsaved changes modal?

Good point. This was not requested as part of this ticket, if needed I would do it in a separate task.

@jesusreal
Copy link
Contributor

It does not work properly in preserveView/goBack scenario: when you are on a page where there is a go back possibility with preserved view and you try to go back using linkManager().goBack() and the current page is dirty the content goes back to the preserved view, the URL stays the same, and the unsaved changes modal pops up. Independent of what you choose (Yes or No) the situation won't get better.

done

@jesusreal
Copy link
Contributor

Don't we want to have a backdrop for the unsaved changes modal?

Good point. This was not requested as part of this ticket, if needed I would do it in a separate task.

done

…hanges-modal

# Conflicts:
#	core/src/App.html
#	core/src/Authorization.html
#	core/src/navigation/LeftNav.html
#	core/src/navigation/TopNav.html
#	core/src/services/routing.js
#	core/src/utilities/helpers/generic-helpers.js

### navigate

Navigates to the given path in the hosting Luigi application. Contains either a full absolute path or a relative path without a leading slash that uses the active route as a base. This is a standard navigation.
Navigates to the given path in application hosted by Luigi. It contains either a full absolute path or a relative path without a leading slash that uses the active route as a base. This is the standard navigation.
Copy link

Choose a reason for hiding this comment

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

Suggested change
Navigates to the given path in application hosted by Luigi. It contains either a full absolute path or a relative path without a leading slash that uses the active route as a base. This is the standard navigation.
Navigates to the given path in the application hosted by Luigi. It contains either a full absolute path or a relative path without a leading slash that uses the active route as a base. This is the standard navigation.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

LuigiClient.linkManager.fromContext("currentTeam").withParams({foo: "bar"}).navigate("path")
```

Returns **[linkManager](#linkmanager)** link manager instance.

### pathExists

Checks if a path you can navigate to exists in the main application. You can use this helper method to perform actions such as conditional display of a DOM element like a button.
Checks if the path you can navigate to exists in the main application. For example, you can use this helper method conditionally display a DOM element like a button.
Copy link

Choose a reason for hiding this comment

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

Suggested change
Checks if the path you can navigate to exists in the main application. For example, you can use this helper method conditionally display a DOM element like a button.
Checks if the path you can navigate to exists in the main application. For example, you can use this helper method conditionally to display a DOM element like a button.

Copy link
Contributor

Choose a reason for hiding this comment

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

done


### setDirtyStatus

Makes current page dirty or not
Copy link

@kazydek kazydek Dec 18, 2018

Choose a reason for hiding this comment

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

Suggested change
Makes current page dirty or not
Makes the current page dirty or not.

Copy link

Choose a reason for hiding this comment

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

Could you explain in more details what is meant in this sentence? As for the periods at the end of the sentence and starting it with the verb without a subject I am against but added it here to keep the consistency in the whole text as I can see the rest starts and end similarly.

Copy link
Contributor

@jesusreal jesusreal Dec 18, 2018

Choose a reason for hiding this comment

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

This method informs the main application that the current page or component in the iframe is dirty, which means it has unsaved changes, like for example a page with form fields which has been modified from their original values.

Copy link
Contributor

Choose a reason for hiding this comment

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

done


#### Parameters

- `isDirty` **[boolean](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Boolean)** tells if current page/component has any unsaved changes at the moment
Copy link

@kazydek kazydek Dec 18, 2018

Choose a reason for hiding this comment

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

Suggested change
- `isDirty` **[boolean](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Boolean)** tells if current page/component has any unsaved changes at the moment
- `isDirty` **[boolean](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Boolean)** tells if the current page or component has any unsaved changes

Copy link
Contributor

Choose a reason for hiding this comment

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

done


### setDirtyStatus

This method informs the main application that the current view in the iframe has unsaved changes, like for example a view with form fields which have been edited but not yet submitted.
Copy link

Choose a reason for hiding this comment

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

Suggested change
This method informs the main application that the current view in the iframe has unsaved changes, like for example a view with form fields which have been edited but not yet submitted.
This method informs the main application that there are unsaved changes in the current view in the iframe. For example, that can be a view with form fields which were edited but not submitted.

Copy link
Contributor

Choose a reason for hiding this comment

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

done


#### Parameters

- `isDirty` **[boolean](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Boolean)** tells if the current page or component has any unsaved changes at the moment
Copy link

Choose a reason for hiding this comment

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

Suggested change
- `isDirty` **[boolean](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Boolean)** tells if the current page or component has any unsaved changes at the moment
- `isDirty` **[boolean](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Boolean)** tells if there are any unsaved changes on the current page or component

@jesusreal jesusreal merged commit 778e691 into SAP:master Dec 18, 2018
@parostatkiem-zz parostatkiem-zz deleted the Unsaved-changes-modal branch February 5, 2019 08:29
stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants