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.

Sign up for GitHub

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 Json-editor multiple instance #894

Merged
merged 6 commits into from
Feb 4, 2021
Merged

Fix Json-editor multiple instance #894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ed434dc
add dynamic ref props with default value
Njuelle Jan 27, 2021
db7afd9
add e2e tests
Njuelle Jan 27, 2021
73f1f62
fix tests
Njuelle Jan 27, 2021
7cb0d18
Merge branch '4-dev' into 893-fix-jsonFormInput
Njuelle Jan 27, 2021
3dca4dc
fix tests
Njuelle Jan 27, 2021
50c6d67
lint
Njuelle Jan 29, 2021
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
38 changes: 23 additions & 15 deletions src/components/Common/JsonEditor.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
<template>
<div
:name="id"
data-cy="JSONEditor"
ref="jsoneditor"
:ref="id"
:class="classes"
:id="id"
:style="style"
Expand All @@ -27,20 +28,27 @@

<script>
import Vue from 'vue'
let editor

export default {
name: 'JsonEditor',
props: {
content: String,
id: {
type: String,
default: Date.now().toString() + Math.random().toString()
},
myclass: {
type: String,
default: ''
},
readonly: Boolean,
id: String,
height: { type: Number, default: 250 }
},
data() {
return {
editor: null
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of putting the editor variable in the component state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is necessary to have the editor variable scoped to the component to be able to use several instances

Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor

@berthieresteban berthieresteban Feb 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If we let the editor in global:

Hypothesis: if we add multiple json-editors there will still be only one editor variable wich will be overrided each time we instanciate a new json-editor.

Actual consequence: Call the setContent method of a json-editor component will always set the content on the last instanciated editor no matter wich json-editor's method we call

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. The editor variable is scoped to the JSONEditor.vue module, which is a singleton, in the CommonJS modules model. The module exports a VueJS Component config, which is passed to the Vue constructor each time this component is instantiated. But this config has a reference to the editor variable, which is actually overwritten, as you rightly say, each time the component is instantiated. By putting the variable inside the data structure, you make it local to the component instead, which is obviously not a singleton. This is a nice thing to keep in mind for future developments.

I still don't like the idea of putting a complex object (i.e. one that has attributes and functions) inside the data structure, because it is meant to keep track of the state of the component, yet this object is just logic to make the editor work. We should find a way to replicate this reference each time the component is created, and avoid the reference to be overwritten.

I am not really hurried to ship this fix, but if you are, we can keep this enhancement for later. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

After merge this PR I'll create an issue concerning the refacto of the json-editor and we'll probably need a workshop

}
},
computed: {
classes() {
return (this.readonly ? 'readonly ' : '') + this.myclass
Expand All @@ -55,40 +63,40 @@ export default {
},
methods: {
getRawValue() {
return editor.getValue()
return this.editor.getValue()
},
getEditor() {
return editor
return this.editor
},
setContent(value) {
this.$log.debug('Setting content', value)
editor.getSession().setValue(value)
this.editor.getSession().setValue(value)
}
},
mounted() {
Vue.nextTick(() => {
/* eslint no-undef: 0 */
editor = ace.edit(this.$refs.jsoneditor, {
this.editor = ace.edit(this.$refs[this.id], {
mode: 'ace/mode/json'
})
editor.setTheme('ace/theme/tomorrow')
editor.setFontSize(15)
editor.getSession().setTabSize(2)
editor.setReadOnly(this.readonly)
editor.$blockScrolling = Infinity
this.editor.setTheme('ace/theme/tomorrow')
this.editor.setFontSize(15)
this.editor.getSession().setTabSize(2)
this.editor.setReadOnly(this.readonly)
this.editor.$blockScrolling = Infinity
this.setContent(this.content)

// WARNING - Beware of update loops!
// This event is triggered both when the content changes after
// user interaction and when it is set programmatically.
editor.on('change', () => {
this.editor.on('change', () => {
this.$emit('change', this.getRawValue())
})
})
},
beforeDestroy() {
if (editor) {
editor.removeAllListeners('change')
if (this.editor) {
this.editor.removeAllListeners('change')
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/components/Data/Documents/FormInputs/JsonFormInput.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<b-col lg="8">
<json-editor
class="h-100"
:id="schema.label"
:content="JSON.stringify(value, null, 2) || '{}'"
@change="onChange"
/>
Expand Down
46 changes: 36 additions & 10 deletions test/e2e/cypress/integration/single-backend/formView.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ describe('Form view', function() {
items: {
type: 'object'
},
skill: {
properties: {
name: { type: 'keyword' },
level: { type: 'integer' }
}
},
job: {
type: 'text'
},
Expand All @@ -38,6 +44,10 @@ describe('Form view', function() {
phone: 'Blackberry',
car: 'Laguna'
},
skill: {
name: 'managment',
level: '1'
},
job: 'Always asking Esteban to do his job',
employeeOfTheMonthSince: '1996-07-10'
}
Comment on lines 44 to 53
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Expand All @@ -49,15 +59,14 @@ describe('Form view', function() {
it('should be able to create a new document with the form view enabled', function() {
cy.visit(`/#/data/${indexName}/${collectionName}`)


cy.get('[data-cy="CreateDocument-btn"').click()


cy.get('[data-cy="formView-switch"').click({ force: true })
cy.get('[data-cy="DocumentCreate-input--id"').type('new-doc')

cy.get('input#age').type('31')
cy.get('textarea.ace_text-input')

cy.get('[name="items"] > textarea.ace_text-input')
.type('{backspace}{backspace}', { force: true })
.type(
`{
Expand All @@ -66,6 +75,17 @@ describe('Form view', function() {
force: true
}
)

cy.get('[name="skill"] > textarea.ace_text-input')
.type('{backspace}{backspace}', { force: true })
.type(
`{
"name": "CSS", "level": 60`,
{
force: true
}
)

cy.get('textarea#job').type('webmestre enginer')
cy.get('input#name').type('Bombi')

Expand All @@ -74,7 +94,6 @@ describe('Form view', function() {

cy.get('[data-cy="DocumentCreate-btn"').click({ force: true })


cy.contains('new-doc')
cy.request(
'GET',
Expand All @@ -86,16 +105,16 @@ describe('Form view', function() {
date.getTime().toString()
)
expect(res.body.result._source.items.desktop).to.be.equals('standing')
expect(res.body.result._source.skill.name).to.be.equals('CSS')
expect(res.body.result._source.skill.level).to.be.equals(60)
})
})

it('should be able to update a document with the form view enabled', function() {
cy.visit(`/#/data/${indexName}/${collectionName}`)


cy.get('[data-cy="DocumentListItem-update--testdoc"').click()


cy.get('[data-cy="formView-switch"').click({ force: true })

cy.get('input#age').type('{selectall}{backspace}43')
Expand All @@ -107,6 +126,16 @@ describe('Form view', function() {
.clear()
.type('23:30:00')

cy.get('[name="skill"] > textarea.ace_text-input').type(
`{selectall}{backspace}
{
"name": "management", "level": 0`,
{
delay: 400,
force: true
}
)

cy.get('[data-cy="DocumentUpdate-btn"').click({ force: true })

cy.request(
Expand All @@ -119,16 +148,15 @@ describe('Form view', function() {
expect(res.body.result._source.employeeOfTheMonthSince).to.be.equals(
date.getTime().toString()
)
expect(res.body.result._source.skill.level).to.be.equals(0)
})
})

it('should be able to keep synchronized the form view and the JSON view', function() {
cy.visit(`/#/data/${indexName}/${collectionName}`)


cy.get('[data-cy="DocumentListItem-update--testdoc"').click()


cy.get('textarea.ace_text-input')
.type(`{selectall}{backspace}`, {
delay: 400,
Expand Down Expand Up @@ -159,10 +187,8 @@ describe('Form view', function() {
)
cy.visit(`/#/data/${indexName}/${collectionName}`)


cy.get('[data-cy="DocumentListItem-update--witharraydoc"').click()


cy.get('[data-cy="formView-switch"').click({ force: true })

cy.get('[data-cy="form-view-warning"').should('be.visible')
Expand Down