Fix Modals #660
Fix Modals #660
Conversation
| @@ -5,74 +5,95 @@ | |||
| </template> | |||
| <script> | |||
| import sbp from '~/shared/sbp.js' | |||
| import { OPEN_MODAL, LOAD_MODAL, UNLOAD_MODAL, REPLACE_MODAL, CLOSE_MODAL } from '@utils/events.js' | |||
| import { OPEN_MODAL, REPLACE_MODAL, UNLOAD_MODAL, CLOSE_MODAL } from '@utils/events.js' | |||
| let keyboardEvent = Event | |||
There was a problem hiding this comment.
couldn't we place this inside the component as a method? Then we would just need to do this:
created () {
window.addEventListener('keyup', this.handleKeyUp)
},
beforeDestroy() {
window.removeEventListener('keyup', this.handleKeyUp)
},
methods: {
handleKeyUp (e) {
// Only if there an active modal
if (this.content && e.key === 'Escape') {
this.unloadModal()
}
}
}| if (this.content) { | ||
| this.$router.push({ query: { modal: this.content, subcontent: this.activeSubcontent() } }) | ||
| } else { | ||
| this.$router.push({}) |
There was a problem hiding this comment.
I believe we shouldn't use push({}), because this removes all the queries from an URL. For example:
http://localhost:8000/app/?id=special&modal=SignUp
when the modal is closed, the other queries are also removed
http://localhost:8000/app/
And maybe we might want to keep the other queries... I can't point any existing example at the moment but in other projects it's a common constraint I had to handle, so I just wanted to remind you of that.
There was a problem hiding this comment.
Hello @pieer ! This is almost perfect!
Others:
- Closing / canceling the change password closes the user settings modal as well - it should only close the first one.
Let me know if you have any questions!
| content: null, | ||
| subcontent: [] | ||
| content: null, // This is the main modal | ||
| subcontent: [] // This is for collection of modal on top of modals |
There was a problem hiding this comment.
It seems like there might be opportunity here for simplification too?
Is it possible to only have an array of content? I.e. make it be content: [] instead of content: null, and remove the subcontent?
There was a problem hiding this comment.
I'm not sure how it would simplify, do you want to change the url structure?
|
@pieer everything is looking perfect! Great work 😎 |
No description provided.