From 27ae08c7ef1d28574b95d924ed9ea70dc188e2e4 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Thu, 28 Sep 2023 07:04:14 +0200 Subject: [PATCH 1/4] Handle sparse positions correctly when adding sections We do not consolidate positions when removing sections to prevent failing requests while deleting chapters. Collection length can, thus, not be used to ensure unique position attributes. REDMINE-20217 --- .../package/spec/editor/models/Chapter-spec.js | 12 ++++++++++++ .../scrolled/package/src/editor/models/Chapter.js | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/entry_types/scrolled/package/spec/editor/models/Chapter-spec.js b/entry_types/scrolled/package/spec/editor/models/Chapter-spec.js index 954d52aa2..022e34064 100644 --- a/entry_types/scrolled/package/spec/editor/models/Chapter-spec.js +++ b/entry_types/scrolled/package/spec/editor/models/Chapter-spec.js @@ -31,6 +31,18 @@ describe('Chapter', () => { expect(section.configuration.get('transition')).toEqual('beforeAfter'); }); + + it('handles sparse positions correctly', () => { + const {entry} = testContext; + + const chapter = entry.chapters.first() + const firstSection = chapter.addSection(); + chapter.addSection(); + firstSection.destroy(); + chapter.addSection(); + + expect(chapter.sections.pluck('position')).toEqual([1, 2]); + }); }); describe('#insertSection', () => { diff --git a/entry_types/scrolled/package/src/editor/models/Chapter.js b/entry_types/scrolled/package/src/editor/models/Chapter.js index 677890adc..34798b353 100644 --- a/entry_types/scrolled/package/src/editor/models/Chapter.js +++ b/entry_types/scrolled/package/src/editor/models/Chapter.js @@ -36,7 +36,7 @@ export const Chapter = Backbone.Model.extend({ const section = this.sections.create( new Section( { - position: this.sections.length, + position: this.sections.length ? Math.max(...this.sections.pluck('position')) + 1 : 0, chapterId: this.id, configuration: { transition: this.entry.metadata.configuration.get('defaultTransition') From cd14c7b07fb7fe4a90aa42dde266faa17712b81e Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Thu, 28 Sep 2023 07:07:14 +0200 Subject: [PATCH 2/4] Extract Section#getTransition from SectionItemView REDMINE-20217 --- .../spec/editor/models/Section-spec.js | 49 +++++++++++++++++++ .../package/src/editor/models/Section.js | 28 +++++++++++ .../src/editor/views/SectionItemView.js | 27 +++------- 3 files changed, 84 insertions(+), 20 deletions(-) create mode 100644 entry_types/scrolled/package/spec/editor/models/Section-spec.js diff --git a/entry_types/scrolled/package/spec/editor/models/Section-spec.js b/entry_types/scrolled/package/spec/editor/models/Section-spec.js new file mode 100644 index 000000000..1598aad9b --- /dev/null +++ b/entry_types/scrolled/package/spec/editor/models/Section-spec.js @@ -0,0 +1,49 @@ +import {ScrolledEntry} from 'editor/models/ScrolledEntry'; +import {factories} from 'pageflow/testHelpers'; +import {normalizeSeed} from 'support'; + +describe('Section', () => { + describe('getTransition', () => { + it('returns transition from configuration', () => { + const entry = factories.entry(ScrolledEntry, {}, { + entryTypeSeed: normalizeSeed({ + sections: [ + {id: 10, position: 0}, + {id: 11, position: 1, configuration: {transition: 'reveal'}} + ] + }) + }); + const section = entry.sections.get(11); + + expect(section.getTransition()).toEqual('reveal'); + }); + + it('turns fade into scroll if fade transition is not available', () => { + const entry = factories.entry(ScrolledEntry, {}, { + entryTypeSeed: normalizeSeed({ + sections: [ + {id: 10, position: 0, configuration: {fullHeight: false}}, + {id: 11, position: 1, configuration: {transition: 'fade', fullHeight: true}} + ] + }) + }); + const section = entry.sections.get(11); + + expect(section.getTransition()).toEqual('scroll'); + }); + + it('keeps fade if fade transition is available', () => { + const entry = factories.entry(ScrolledEntry, {}, { + entryTypeSeed: normalizeSeed({ + sections: [ + {id: 10, position: 0, configuration: {fullHeight: true}}, + {id: 11, position: 1, configuration: {transition: 'fade', fullHeight: true}} + ] + }) + }); + const section = entry.sections.get(11); + + expect(section.getTransition()).toEqual('fade'); + }); + }); +}); diff --git a/entry_types/scrolled/package/src/editor/models/Section.js b/entry_types/scrolled/package/src/editor/models/Section.js index b81dfe389..652ee4514 100644 --- a/entry_types/scrolled/package/src/editor/models/Section.js +++ b/entry_types/scrolled/package/src/editor/models/Section.js @@ -1,4 +1,5 @@ import Backbone from 'backbone'; +import {getAvailableTransitionNames} from 'pageflow-scrolled/frontend'; import { configurationContainer, @@ -35,4 +36,31 @@ export const Section = Backbone.Model.extend({ chapterPosition: function() { return this.chapter && this.chapter.has('position') ? this.chapter.get('position') : -1; }, + + getTransition() { + const entry = this.chapter?.entry; + + if (!entry) { + return 'scroll'; + } + + const sectionIndex = entry.sections.indexOf(this); + const previousSection = entry.sections.at(sectionIndex - 1); + + const availableTransitions = + previousSection ? + getAvailableTransitionNames( + this.configuration.attributes, + previousSection.configuration.attributes + ) : []; + + const transition = this.configuration.get('transition'); + + if (availableTransitions.includes(transition)) { + return transition; + } + else { + return 'scroll'; + } + } }); diff --git a/entry_types/scrolled/package/src/editor/views/SectionItemView.js b/entry_types/scrolled/package/src/editor/views/SectionItemView.js index 097a7717d..f99add0b7 100644 --- a/entry_types/scrolled/package/src/editor/views/SectionItemView.js +++ b/entry_types/scrolled/package/src/editor/views/SectionItemView.js @@ -71,6 +71,8 @@ export const SectionItemView = Marionette.ItemView.extend({ }, onRender() { + this.updateTransition(); + if (this.updateActive()) { setTimeout(() => this.$el[0].scrollIntoView({block: 'nearest'}), 10) } @@ -118,26 +120,11 @@ export const SectionItemView = Marionette.ItemView.extend({ }), {to: this.ui.dropDownButton}); }, - getTransition() { - const entry = this.options.entry; - const sectionIndex = entry.sections.indexOf(this.model); - const previousSection = entry.sections.at(sectionIndex - 1); - - const availableTransitions = - previousSection ? - getAvailableTransitionNames( - this.model.configuration.attributes, - previousSection.configuration.attributes - ) : []; - - const transition = this.model.configuration.get('transition'); - - if (availableTransitions.includes(transition)) { - return transition; - } - else { - return 'scroll'; - } + updateTransition() { + this.ui.transition.text( + I18n.t(this.model.getTransition(), + {scope: 'pageflow_scrolled.editor.section_item.transitions'}) + ); }, updateActive() { From 94bd7481269b88c84cef3e20ace62557b3f4ed50 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Thu, 28 Sep 2023 07:07:56 +0200 Subject: [PATCH 3/4] Ensure parent chapter is set on section before initial sort Chapter positions are needed to sort the global sections collection. Ensure parent model is already set during initial sort, such that previous sections can be determined correctly right from the start. Otherwise, the outline displays an incorrect transition after inserting a section since the previous section can not yet be used to determine available transitions. REDMINE-20217 --- .../ForeignKeySubsetCollection-spec.js | 18 ++++++++++++++++++ .../collections/ForeignKeySubsetCollection.js | 16 ++++++++-------- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/package/spec/editor/collections/ForeignKeySubsetCollection-spec.js b/package/spec/editor/collections/ForeignKeySubsetCollection-spec.js index 499206193..cd56830d2 100644 --- a/package/spec/editor/collections/ForeignKeySubsetCollection-spec.js +++ b/package/spec/editor/collections/ForeignKeySubsetCollection-spec.js @@ -180,6 +180,24 @@ describe('ForeignKeySubsetCollection', () => { expect(postComments.last().post).toBe(post); }); + it('sets reference before intial sort of parent', () => { + const post = new Backbone.Model({id: 5}); + const comments = new Backbone.Collection([ + ], {comparator: 'position'}); + const postComments = new ForeignKeySubsetCollection({ + parentModel: post, + parent: comments, + foreignKeyAttribute: 'postId', + parentReferenceAttribute: 'post' + }); + let referenceOnInitialSort; + + comments.once('sort', () => referenceOnInitialSort = comments.first().post); + postComments.add({id: 2}) + + expect(referenceOnInitialSort).toBe(post); + }); + it('removes reference to parent model when model is removed from collection', () => { const post = new Backbone.Model({id: 5}); const comments = new Backbone.Collection([ diff --git a/package/src/editor/collections/ForeignKeySubsetCollection.js b/package/src/editor/collections/ForeignKeySubsetCollection.js index 6939be9da..c32a52597 100644 --- a/package/src/editor/collections/ForeignKeySubsetCollection.js +++ b/package/src/editor/collections/ForeignKeySubsetCollection.js @@ -27,6 +27,14 @@ export const ForeignKeySubsetCollection = SubsetCollection.extend({ this.autoConsolidatePositions = options.autoConsolidatePositions; + this.listenTo(this, 'add', function(model) { + if (options.parentReferenceAttribute) { + model[options.parentReferenceAttribute] = parentModel; + } + + model.set(options.foreignKeyAttribute, parentModel.id); + }); + SubsetCollection.prototype.constructor.call(this, { parent, parentModel, @@ -41,14 +49,6 @@ export const ForeignKeySubsetCollection = SubsetCollection.extend({ } }); - this.listenTo(this, 'add', function(model) { - if (options.parentReferenceAttribute) { - model[options.parentReferenceAttribute] = parentModel; - } - - model.set(options.foreignKeyAttribute, parentModel.id); - }); - this.listenTo(parentModel, 'destroy dependentDestroy', function() { this.invoke('trigger', 'dependentDestroy'); this.clear(); From f7da24396edd1f800e8691dd47b4b57ec521fa60 Mon Sep 17 00:00:00 2001 From: Tim Fischbach Date: Thu, 28 Sep 2023 07:10:46 +0200 Subject: [PATCH 4/4] Update section item when inserting sections When inserting a section at the beginning of the entry, we need to update the transition of the previously first section. Also the section might no longer be the active section, even though currentSectionIndex did not change. REDMINE-20217 --- .../scrolled/package/src/editor/views/SectionItemView.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/entry_types/scrolled/package/src/editor/views/SectionItemView.js b/entry_types/scrolled/package/src/editor/views/SectionItemView.js index f99add0b7..61861e865 100644 --- a/entry_types/scrolled/package/src/editor/views/SectionItemView.js +++ b/entry_types/scrolled/package/src/editor/views/SectionItemView.js @@ -68,6 +68,11 @@ export const SectionItemView = Marionette.ItemView.extend({ }); } }); + + this.listenTo(this.options.entry.sections, 'add', () => { + this.updateActive(); + this.updateTransition(); + }); }, onRender() { @@ -78,10 +83,6 @@ export const SectionItemView = Marionette.ItemView.extend({ } this.$el.toggleClass(styles.invert, !!this.model.configuration.get('invert')); - this.ui.transition.text( - I18n.t(this.getTransition(), - {scope: 'pageflow_scrolled.editor.section_item.transitions'}) - ); this.subview(new SectionThumbnailView({ el: this.ui.thumbnail,