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

Fix Modals #660

Merged
merged 24 commits into from
Sep 16, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion frontend/utils/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ export const REPLACED_STATE = 'replaced-state'
export const CONTRACTS_MODIFIED = 'contracts-modified'

export const OPEN_MODAL = 'open-modal'
export const LOAD_MODAL = 'load-modal'
export const CLOSE_MODAL = 'close-modal'
export const UNLOAD_MODAL = 'unload-modal'
pieer marked this conversation as resolved.
Show resolved Hide resolved
export const REPLACE_MODAL = 'replace-modal'
Expand Down
2 changes: 1 addition & 1 deletion frontend/utils/lazyLoadedView.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import Vue from 'vue'
// NOTE: any modals opened with `LOAD_MODAL` go here
// NOTE: any modals opened with `OPEN_MODAL` go here
Vue.component('LoginModal', () => import('../views/containers/modals/Login.vue'))
Vue.component('SignUp', () => import('../views/containers/modals/SignUp.vue'))
Vue.component('Settings', () => import('../views/containers/settings/Wrapper.vue'))
Expand Down
1 change: 0 additions & 1 deletion frontend/views/components/Chatroom/ChatMain.vue
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@ export default {
return this.details.conversation.findIndex(message => message.unread === true)
},
getPendingAt () {
console.log('PLOP')
return this.ephemeral.pendingMessages.map((message, index) => ({
text: message.text,
who: this.who(true),
Expand Down
4 changes: 2 additions & 2 deletions frontend/views/components/GroupSettings.vue
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
<script>
import sbp from '~/shared/sbp.js'
import { toPercent } from '@view-utils/filters.js'
import { LOAD_MODAL } from '@utils/events.js'
import { OPEN_MODAL } from '@utils/events.js'

// TODO: there is another file called GroupSettings in pages/, rename this one (and move it?)
export default {
Expand All @@ -48,7 +48,7 @@ export default {
},
methods: {
openProposal (component) {
sbp('okTurtles.events/emit', LOAD_MODAL, component)
sbp('okTurtles.events/emit', OPEN_MODAL, component)
}
}
}
Expand Down
95 changes: 58 additions & 37 deletions frontend/views/components/Modal/Modal.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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()
      }
    }
  }


export default {
name: 'Modal',
data () {
return {
// QUESTION: What's the dif between content and subcontent?
// What are the use cases of it?
content: null,
subcontent: []
content: null, // This is the main modal
subcontent: [] // This is for collection of modal on top of modals
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure how it would simplify, do you want to change the url structure?

}
},
created () {
sbp('okTurtles.events/on', LOAD_MODAL, component => this.openModal(component))
sbp('okTurtles.events/on', UNLOAD_MODAL, component => this.closeModal(component))
sbp('okTurtles.events/on', OPEN_MODAL, component => this.openModal(component))
sbp('okTurtles.events/on', UNLOAD_MODAL, component => this.unloadModal(component))
sbp('okTurtles.events/on', REPLACE_MODAL, component => this.replaceModal(component))
// When press escape it should close the modal
keyboardEvent = window.addEventListener('keyup', (e) => {
// Only if there an active modal
if (this.content && e.key === 'Escape') {
this.unloadModal()
}
})
},
mounted () {
const modal = this.$route.query.modal
if (modal) this.openModal(modal)
this.initializeModals()
},
beforeDestroy () {
sbp('okTurtles.events/off', LOAD_MODAL, this.openModal)
sbp('okTurtles.events/off', UNLOAD_MODAL, this.closeModal)
sbp('okTurtles.events/off', REPLACE_MODAL, this.replaceModal)
sbp('okTurtles.events/off', OPEN_MODAL)
sbp('okTurtles.events/off', UNLOAD_MODAL)
sbp('okTurtles.events/off', REPLACE_MODAL)
window.removeEventListener('keyup', keyboardEvent)
},
watch: {
'$route' (to, from) {
if (from.query.modal && !to.query.modal) {
sbp('okTurtles.events/emit', CLOSE_MODAL)
if (to.query.modal) {
// We reset the modals with no animation for simplicity
if (to.query.modal !== this.content) this.content = to.query.modal
const subcontent = to.query.subcontent
if (subcontent && subcontent !== this.activeSubcontent()) {
// Try to find the new subcontent in the list of subcontent
const i = this.subcontent.indexOf(subcontent)
if (i) this.subcontent = this.subcontent.splice(0, i)
else this.subcontent = subcontent
}
} else {
// When the route change we compare to see if the modal changed
// If so, we send the event to close the modal
if (from.query.modal) {
sbp('okTurtles.events/emit', CLOSE_MODAL)
}
}
}
},
methods: {
activeSubcontent () {
return this.subcontent[this.subcontent.length - 1]
},
initializeModals () {
const modal = this.$route.query.modal
if (modal) this.openModal(modal)
const subcontent = this.$route.query.subcontent
if (subcontent) this.openModal(subcontent)
},
updateUrl () {
if (this.content) {
this.$router.push({ query: { modal: this.content, subcontent: this.activeSubcontent() } })
} else {
this.$router.push({})
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
},
openModal (componentName) {
if (this.content) {
this.subcontent.push(componentName)
// QUESTION: Why emmiting this event if
// there is not any events/on' poiting to OPEN_MODAL?
sbp('okTurtles.events/emit', OPEN_MODAL)
} else {
this.content = componentName
// QUESTION: Same here..
sbp('okTurtles.events/emit', OPEN_MODAL)
}
this.$router.push({ query: { modal: this.content, subcontent: this.subcontent[this.subcontent.length - 1] } })
},
replaceModal (componentName) {
this.closeModal()
setTimeout(() => {
this.openModal(componentName)
}, 300)
this.updateUrl()
},
closeModal () {
const query = this.$route.query || {}

unloadModal (name) {
if (this.subcontent.length) {
this.subcontent.pop()
query.subcontent = this.subcontent[this.subcontent.length - 1]
query.modal = this.content
} else {
this.content = undefined
this.content = null
}

// BUG: This isn't working at all but if you pause the dev
// tools and do it manually on the console it works :/
// Avoid event problem by removing completly the component
this.$router.push({ query: query })
this.updateUrl()
},
replaceModal (componentName) {
this.unloadModal()
this.openModal(componentName)
}
}
}
Expand Down
13 changes: 11 additions & 2 deletions frontend/views/components/Modal/ModalBaseTemplate.vue
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
<template lang='pug'>
transition(name='zoom' appear @after-leave='$destroy()')
sandrina-p marked this conversation as resolved.
Show resolved Hide resolved
transition(name='zoom' appear)
.modal(data-test='modal' role='dialog' v-if='isActive')
modal-close(@close='close')
.has-background
modal-close(@close='close')
slot
</template>

Expand All @@ -15,6 +16,8 @@ export default {
</script>

<style lang='scss' scoped>
@import "../../../assets/style/_variables.scss";

.modal {
display: flex;
position: fixed;
Expand All @@ -29,4 +32,10 @@ export default {
.modal-body {
height: 100%;
}

.has-background .c-modal-close {
@include tablet {
background-color: $background;
}
}
</style>
21 changes: 8 additions & 13 deletions frontend/views/components/Modal/ModalClose.vue
Original file line number Diff line number Diff line change
Expand Up @@ -16,32 +16,27 @@ export default {

.c-modal-close {
position: absolute;
right: 1rem;
top: 1.375rem;
right: $spacer;
top: $spacer;
z-index: 4;
height: 32px;
min-height: 32px;
width: 32px;
height: 2.75rem;
width: 2.75rem;
border: none;
border-radius: 50%;
padding: 0;
@extend %unselectable;
-moz-appearance: none;
-webkit-appearance: none;
cursor: pointer;
background-color: $general_1;

@include tablet {
background-color: $background;
top: 24px;
right: 24px;
height: 40px;
width: 40px;
top: 1.5rem;
right: 1.5rem;
}

@include desktop {
top: 1rem;
right: 1rem;
top: $spacer;
right: $spacer;
}

&::before,
Expand Down
32 changes: 5 additions & 27 deletions frontend/views/components/Modal/ModalMixins.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import sbp from '~/shared/sbp.js'
import { CLOSE_MODAL, UNLOAD_MODAL } from '~/frontend/utils/events.js'
import ModalClose from './ModalClose.vue'
let keyboardEvent = Event

const modaMixins = {
data () {
Expand All @@ -13,38 +12,17 @@ const modaMixins = {
ModalClose
},
mounted () {
sbp('okTurtles.events/on', CLOSE_MODAL, this.hide)
keyboardEvent = window.addEventListener('keyup', (e) => {
if (e.key === 'Escape') {
this.close()
}
})
sbp('okTurtles.events/on', CLOSE_MODAL, this.close)
},
beforeDestroy () {
sbp('okTurtles.events/emit', UNLOAD_MODAL)
window.removeEventListener('keyup', keyboardEvent)
sbp('okTurtles.events/off', CLOSE_MODAL)
},
methods: {
close (e) {
sbp('okTurtles.events/emit', CLOSE_MODAL)
},
hide () {
// Only the last open modal should close
// TODO: find a more eleguant way to identify the current instance
// Maybe saving the name in the modal ?
const instance = this.$parent.$options._componentTag
const query = this.$route.query
if (query) {
const subcontent = query.subcontent
if (subcontent) {
if (subcontent === instance) this.isActive = false
// Don't do anything if it's not the last modal
return false
}
}
// Remove event only if it's the last modal
sbp('okTurtles.events/off', CLOSE_MODAL)
this.isActive = false
setTimeout(() => {
sbp('okTurtles.events/emit', UNLOAD_MODAL)
}, 300)
pieer marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
20 changes: 10 additions & 10 deletions frontend/views/components/Modal/ModalTemplate.vue
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
transition(name='fade' appear)
.c-modal-background(@click='close' v-if='isActive')

transition(name='slide-left' appear @after-leave='$destroy()')
transition(name='slide-left' appear)
.c-modal-content(ref='card' v-if='isActive')
header.c-modal-header(
:class='{ "has-subtitle": $scopedSlots.subtitle }'
Expand Down Expand Up @@ -108,26 +108,26 @@ export default {

.c-modal-header {
position: relative;
align-items: center;
padding: 0 $spacer;
// min-height: 88px;
min-height: 60px;
min-height: 4.75ren;
pieer marked this conversation as resolved.
Show resolved Hide resolved

@include tablet {
min-height: 95px;
align-items: center;
min-height: 5.75rem;
align-items: center;
pieer marked this conversation as resolved.
Show resolved Hide resolved
}

@include tablet {
min-height: 105px;
@include desktop {
min-height: 6.5rem;
}

&.has-subtitle {
min-height: 5.625rem;
@include tablet {
min-height: 114px;
min-height: 6.625rem;
}
@include desktop {
min-height: 124px;
min-height: 7.5rem;
}
}

Expand All @@ -137,7 +137,7 @@ export default {
}

.c-modal-body {
margin: $spacer $spacer 2.5rem $spacer;
margin: $spacer $spacer $spacer-lg $spacer;

&:last-child {
padding-bottom: $spacer-lg;
pieer marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
8 changes: 4 additions & 4 deletions frontend/views/components/Modal/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ Let's see a real working example: **the Login modal**:

### How to open a modal

From anywhere in the App we can open any modal by using the `sbp` event `LOAD_MODAL` with the Vue Component that contains the modal Content
From anywhere in the App we can open any modal by using the `sbp` event `OPEN_MODAL` with the Vue Component that contains the modal Content

```html
<!-- NavBar.vue -->
Expand All @@ -18,10 +18,10 @@ From anywhere in the App we can open any modal by using the `sbp` event `LOAD_MO

<script>
import sbp from '~/shared/sbp.js'
import { LOAD_MODAL } from '@utils/events.js'
import { OPEN_MODAL } from '@utils/events.js'

openLoginModal () {
sbp('okTurtles.events/emit', LOAD_MODAL, 'LoginModal')
sbp('okTurtles.events/emit', OPEN_MODAL, 'LoginModal')
// or pass an object with 'name' and 'subfolder' properties relative to containers folder // TODO: explain this better?
}
</script>
Expand All @@ -42,7 +42,7 @@ closeModal () {

The `<modal />` is imported at `simple/index.html` at the bottom of the DOM.

Its content use <modal-template /> and is dynamically loaded [(Know more about Vue Dynamic Components)](https://vuejs.org/v2/api/#is). The modal use `sbp` events `LOAD_MODAL` and `CLOSE_MODAL`.
Its content use <modal-template /> and is dynamically loaded [(Know more about Vue Dynamic Components)](https://vuejs.org/v2/api/#is). The modal use `sbp` events `OPEN_MODAL` and `CLOSE_MODAL`.


### The `<modal-template />`
Expand Down
Loading