Unify modals #1704
Unify modals #1704
Conversation
…whether we are connected or not
index.html
Outdated
|
|
||
| <div id="contentFrame"> | ||
| <!-- overlay is used by pageNav drop down menus and should be here i the markup so that it | ||
| properly aboscures any open modals (without having to manage z-indexes) --> |
There was a problem hiding this comment.
Minor spelling mistakes.
i = in
aboscures = obscures
|
|
||
| options = options || {}; | ||
| options.className = 'modal modal-opaque ' + __.result(this, 'className', '') + | ||
| options = __.extend({}, defaults, options || {}); |
There was a problem hiding this comment.
Would
options = __.extend(options || {}, defaults);
be more clear?
There was a problem hiding this comment.
Nevermind, options needs to be second to override defaults.
| }; | ||
|
|
||
| app.router = new router({userModel: user, userProfile: userProfile, socketView: newSocketView}); | ||
|
|
There was a problem hiding this comment.
When launchOnboarding creates a new onboardingModal, shouldn't it get
showCloseButton: false,
dismissOnOverlayClick: false,
dismissOnEscPress: false
As options? Being able to close the onboarding modal (by accident or on purpose) creates a weird experience where the loading screen appears briefly then the onboarding modal reappears.
There should probably be a startUpLoadingModal.close(); in launchOnboarding too, otherwise it's hanging around behind the onboarding modal, and can be closed by pressing Esc, which causes the screen to get lighter (because its overlay lightens the screen).
There was a problem hiding this comment.
great catches! I've fixed them.
Also, by default I've made the LoadingModal also get those options preventing it from being closable, since I don't see a scenario where we'd want the user to close it out.
A cool improvement would be to make a 'closeable' option that controls those three, which would simplify the syntax, but I think for now it's ok.
| this.loadingModal.close(); | ||
|
|
||
| this.initAccordion('.js-profileAccordion'); | ||
|
|
There was a problem hiding this comment.
Line 250 should be set to make the guidStillCreatingModal non-closable, otherwise if the guid takes a long time you can close that modal, which causes issues.
I think this would do it:
this.guidStillCreatingModal = new guidStillCreatingModal({
dismissOnOverlayClick: false,
dismissOnEscPress: false,
showCloseButton: false
});
| $('#loadingModal').addClass('hide'); | ||
| config = __.extend({}, defaults, config); | ||
|
|
||
| this.pageConnectModal && this.pageConnectModal.remove(); |
There was a problem hiding this comment.
Right below this, the new pageConnectModal should get
dismissOnEscPress: false
as an option, otherwise you can close it with the esc key, which leaves a blank content area.
There was a problem hiding this comment.
Or esc should do the same as the cancel button, though that's more complicated.
2 participants
This pull request adjusts all modal to extend from the Base Modal.