-
Notifications
You must be signed in to change notification settings - Fork 171
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
Add generic confirmation modal #389
Changes from 12 commits
fe3eb92
240fcd9
ac5e8b4
0998519
7f5476e
e9eca1c
0c37cf7
748583b
9887bbd
eabb55d
88da0df
40c9f82
1ea9034
761c132
ecd0752
080368c
93a4931
ed3fc89
5276171
ae183e8
d7deb89
9f6f5cb
cd6e607
89e4701
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,7 +10,9 @@ var _onContextUpdatedFns = {}; | |||||
var _onInitFns = {}; | ||||||
var authData = {}; | ||||||
var pathExistsPromises = {}; | ||||||
|
||||||
let promises = { | ||||||
confirmationModal: {} | ||||||
}; | ||||||
/** | ||||||
* Creates a random Id | ||||||
* @private | ||||||
|
@@ -101,6 +103,13 @@ function luigiClientInit() { | |||||
pathExistsPromises[data.correlationId].resolveFn(data.pathExists); | ||||||
delete pathExistsPromises[data.correlationId]; | ||||||
} | ||||||
|
||||||
if ('luigi.ux.confirmationModal.hide' === e.data.msg) { | ||||||
const data = e.data.data; | ||||||
const promise = promises.confirmationModal; | ||||||
data.confirmed ? promise.resolveFn() : promise.rejectFn(); | ||||||
delete promises.confirmationModal; | ||||||
} | ||||||
}); | ||||||
|
||||||
window.parent.postMessage({ msg: 'luigi.get-context' }, '*'); | ||||||
|
@@ -410,6 +419,30 @@ const LuigiClient = { | |||||
{ msg: 'luigi.set-page-dirty', dirty: isDirty }, | ||||||
'*' | ||||||
); | ||||||
}, | ||||||
/** | ||||||
* Shows a confirmation modal. | ||||||
* @param {Object} content the content of the confirmation modal | ||||||
* @param {string} content.header text for modal header | ||||||
jesusreal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
* @param {string} content.body text for modal body | ||||||
jesusreal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
* @param {string} content.buttonConfirm text for modal confirm button | ||||||
jesusreal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
* @param {string} content.buttonDismiss text for modal dismiss button | ||||||
jesusreal marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
* @returns {promise} A promise which is resolved when accepting confirmation modal and rejected when dismissing it. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not correct regarding documentation. First comes the type of the returned value and the the additional info. I will just add the "the" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok :) I was just looking at the entire document, and under Also let's then write There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The piece of documentation you mentioned is:
I will replace the line we are talking about with:
This makes it consistent imho. Is that ok for you? |
||||||
*/ | ||||||
showConfirmationModal: function showConfirmationModal(content) { | ||||||
window.parent.postMessage( | ||||||
{ | ||||||
msg: 'luigi.ux.confirmation-modal-show', | ||||||
data: { content } | ||||||
}, | ||||||
'*' | ||||||
); | ||||||
promises.confirmationModal = {}; | ||||||
promises.confirmationModal.promise = new Promise((resolve, reject) => { | ||||||
promises.confirmationModal.resolveFn = resolve; | ||||||
promises.confirmationModal.rejectFn = reject; | ||||||
}); | ||||||
return promises.confirmationModal.promise; | ||||||
} | ||||||
}; | ||||||
} | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need to make it an object inside an object? Why not just
confirmationModalPromise
object?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created it having in mind a future refactoring to include all promises in same object. Apart from this one, we have pathExists promise, and we will soon have generic alert promise. This solution scales better in my opinion