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

[Prototype] Backbone Sub Vue #5308

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/core/Router.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ module.exports = class CocoRouter extends Backbone.Router
console.debug("Skipping route in Backbone - delegating to Vue app")
return
else if options.vueRoute # Routing to a vue component using VueComponentView
vueComponentView = require 'views/core/VueComponentView'
vueComponentView = require 'app/views/core/VueComponentView'
view = new vueComponentView(ViewClass.default, options, args...)
else
view = new ViewClass(options, args...) # options, then any path fragment args
Expand Down
55 changes: 55 additions & 0 deletions app/views/core/SubVueComponentView.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import CocoView from 'views/core/CocoView'
import store from 'core/store'

const silentStore = { commit: _.noop, dispatch: _.noop }

export default class VueComponentView extends CocoView {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix name

constructor (options) {
super(options)

if (!this.VueComponent) {
throw new Error('Class must be initialized with VueComponent')
}

this.props = {}
Copy link
Contributor

@shubhi1092 shubhi1092 May 9, 2019

Choose a reason for hiding this comment

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

Should we set some initial values to the state here in the constructor, just like in VueComponentView

}

getMountPoint() {
return this.$el[0]
}

buildVueComponent() {
return new this.VueComponent({
el: this.getMountPoint(),
propsData: this.props,
store: store
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want to optionally be able to register a functional store? Could this be passed in via the constructor?

})
}

setState (state = {}) {
for (const key of Object.keys(state)) {
Vue.set(
this.props,
key,
state[key]
)
}
}

afterRender () {
if (!this.vueComponent) {
this.vueComponent = this.buildVueComponent()
}

this.props = this.vueComponent.$props

super.afterRender()
}

destroy() {
super.destroy()

this.vueComponent.$destroy()
this.vueComponent.$store = silentStore
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure how much $destroy does. Would we also want to do a this.vueComponent = null?

This is a great backbone interface with Vue.

}
}
26 changes: 20 additions & 6 deletions app/views/core/VueComponentView.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

// This is the base backbone view to render any VueComponent.
// Pass the vue component, propsData, base template into the constructor

Expand All @@ -10,14 +9,16 @@ const silentStore = { commit: _.noop, dispatch: _.noop }
module.exports = class VueComponentView extends RootView {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

VueComponnetView and SubVueCompnentView are close to identical in terms of functionality with the exception that they inherit from different base classes and thus render differently. I don't see a way to avoid having two of them but couldn't think of an easy way to share functionality outside of a mixin and didn't want to spend the time doing that for a prototype.

If we move forward with this we should discuss ways to consolidate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that both these are identical and could possibly be consolidated. I would like to understand the rationale behind inheriting from CocoView instead of RootView for SubVueCompnentView

constructor (component, options) {
super(options)

const baseTemplate = options.baseTemplate || 'base-flat' //base template, by default using base-flat
this.id = 'vue-component-view'

try {
this.template = require('templates/'+ baseTemplate)
}
catch (err) {
} catch (err) {
console.error("Error in importing the base template.", err)
}

this.VueComponent = component
this.propsData = options.propsData
}
Expand All @@ -30,19 +31,32 @@ module.exports = class VueComponentView extends RootView {
})
}

setState (state = {}) {
for (const key of Object.keys(state)) {
Vue.set(
this.props,
key,
state[key]
)
}
}

afterRender() {
if (this.vueComponent) {
this.$el.find('#site-content-area').replaceWith(this.vueComponent.$el)
}
else{
} else {
this.vueComponent = this.buildVueComponent()
window.rootComponent = this.vueComponent
}

this.propsData = this.vueComponent.$props

super.afterRender()
}

destroy() {
destroy () {
super.destroy()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we should add this on master if this PR doesn't eventually get improved / merged into master.


this.vueComponent.$destroy()
this.vueComponent.$store = silentStore
}
Expand Down
44 changes: 30 additions & 14 deletions app/views/play/level/LevelGoals.vue
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<template lang="pug">
div
// TODO: Split this into two components, one the ul, the other the goals-status
// TODO: Split this into two components, one the ul, the other the goals-status
ul#primary-goals-list(dir="auto")
level-goal(
v-for="goal in levelGoals",
Expand All @@ -18,7 +18,7 @@
:goal="goal",
:state="goalStates[goal.id]",
)

div.goals-status.rtl-allowed(v-if="showStatus")
span {{ $t("play_level.goals") }}
span.spr :
Expand All @@ -36,16 +36,32 @@
LevelGoal = require('./LevelGoal').default

module.exports = Vue.extend({
props: ['showStatus']

data: -> {
overallStatus: ''
timedOut: false
goals: [] # TODO: Get goals, goalStates from vuex
goalStates: {}
casting: false
},

props: {
showStatus:
type: Boolean
default: false

overallStatus:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any downside of moving all the state from data to props? From what I know, the vue component should not mutate props and use only the data for mutating values/state. In such a case, would it be a good idea if the Vue component wants to change the state?

type: String
default: ''

timedOut:
type: Boolean
default: false

goals:
type: Array
default: []

goalStates:
type: Object
default: {}

casting:
type: Boolean
default: false
}

computed: {
classToShow: ->
classToShow = 'success' if @overallStatus is 'success'
Expand Down Expand Up @@ -94,15 +110,15 @@
color: rgb(245, 170, 49)
.running
color: rgb(200, 200, 200)

ul
padding-left: 0
margin-bottom: 0
color: black

body[lang="he"] &, body[lang="ar"] &, body[lang="fa"] &, body[lang="ur"] &
padding-right: 0

#concept-goals-list
margin-left: 20px

Expand Down
24 changes: 12 additions & 12 deletions app/views/play/level/LevelGoalsView.coffee
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
require('app/styles/play/level/goals.sass')
CocoView = require 'views/core/CocoView'
SubVueComponentView = require('views/core/SubVueComponentView').default
template = require 'templates/play/level/goals'
{me} = require 'core/auth'
utils = require 'core/utils'
Expand All @@ -10,7 +10,9 @@ LevelGoals = require('./LevelGoals').default
store = require 'core/store'


module.exports = class LevelGoalsView extends CocoView
module.exports = class LevelGoalsView extends SubVueComponentView
VueComponent: LevelGoals

id: 'goals-view'
template: template
className: 'secret expanded'
Expand All @@ -37,17 +39,15 @@ module.exports = class LevelGoalsView extends CocoView
constructor: (options) ->
super options
@level = options.level

afterRender: ->
@levelGoalsComponent = new LevelGoals({
el: @$('.goals-component')[0],
store
propsData: { showStatus: true }
})

@setState({ showStatus: true })
Copy link
Contributor

@shubhi1092 shubhi1092 May 9, 2019

Choose a reason for hiding this comment

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

Related to my constructor comment above, we could set the initial state using the constructor, and then use @setState for changing it afterward


getMountPoint: ->
return @$('.goals-component')[0]

onNewGoalStates: (e) ->
_.assign(@levelGoalsComponent, _.pick(e, 'overallStatus', 'timedOut', 'goals', 'goalStates'))
@levelGoalsComponent.casting = false
@setState(_.pick(e, 'overallStatus', 'timedOut', 'goals', 'goalStates'))
@setState({ casting: false })

firstRun = not @previousGoalStatus?
@previousGoalStatus ?= {}
Expand All @@ -68,7 +68,7 @@ module.exports = class LevelGoalsView extends CocoView

onTomeCast: (e) ->
return if e.preload
@levelGoalsComponent.casting = true
@setState({ casting: true })

onSetPlaying: (e) ->
return unless e.playing
Expand Down